Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Add a new input flag to avoid commenting on stacks that have not changed. #85

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

drew-rsk
Copy link

Adds a flag to allow users to skip comments for stacks that have not changed.

@drew-rsk drew-rsk changed the title Add a new input flag to avoid commenting on stacks that have not changed. feat(): Add a new input flag to avoid commenting on stacks that have not changed. Sep 26, 2024
@drew-rsk drew-rsk changed the title feat(): Add a new input flag to avoid commenting on stacks that have not changed. feat(cdk-diff-action-86): Add a new input flag to avoid commenting on stacks that have not changed. Sep 26, 2024
@drew-rsk drew-rsk changed the title feat(cdk-diff-action-86): Add a new input flag to avoid commenting on stacks that have not changed. feat(cdk-diff-action-3): Add a new input flag to avoid commenting on stacks that have not changed. Sep 26, 2024
@drew-rsk drew-rsk marked this pull request as ready for review September 26, 2024 18:15
@corymhall corymhall enabled auto-merge (squash) September 26, 2024 18:15
auto-merge was automatically disabled September 26, 2024 18:26

Head branch was pushed to by a user without write access

@drew-rsk drew-rsk changed the title feat(cdk-diff-action-3): Add a new input flag to avoid commenting on stacks that have not changed. feat: Add a new input flag to avoid commenting on stacks that have not changed. Sep 26, 2024
Copy link
Owner

@corymhall corymhall left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@drew-rsk overall the PR looks good! The only thing missing is adding the new input property to the action metadata here

actionMetadata: {

I'm a little hesitant to add this feature as is because I'm worried that it creates a scenario where something could fail to process and the user would think that it was because the stack had no changes.

What would you think about having a single summary comment that has info on what stages/stacks were processed? It could just be a single comment saying that stacks x,y,z had no changes.

@drew-rsk
Copy link
Author

drew-rsk commented Sep 30, 2024

@drew-rsk overall the PR looks good! The only thing missing is adding the new input property to the action metadata here

actionMetadata: {

I'm a little hesitant to add this feature as is because I'm worried that it creates a scenario where something could fail to process and the user would think that it was because the stack had no changes.

What would you think about having a single summary comment that has info on what stages/stacks were processed? It could just be a single comment saying that stacks x,y,z had no changes.

@corymhall

Q1: Yeah that'd be a good step and possibly sufficient for my team's development purposes. How can I play with my changes as they are, or any other PR's changes, as a way to manually inspect the results?

Q2: I propose leaving this PR as is and working up an independent one that does what you suggest over hacking this one into a different functional solution. How does that plan suit you?

@corymhall
Copy link
Owner

Q1: Yeah that'd be a good step and possibly sufficient for my team's development purposes. How can I play with my changes as they are, or any other PR's changes, as a way to manually inspect the results?

You can pin an action to a branch if you want to test. Something like this should work.

- name: Diff
        uses: corymhall/cdk-diff-action@drew-rsk:drew/ignore-unchanged-stacks

Q2: I propose leaving this PR as is and working up an independent one that does what you suggest over hacking this one into a different functional solution. How does that plan suit you?

I'm not sure what you are asking here. I'm not comfortable merging this PR as is because I think we need to have some kind of indication that all stacks were processed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants