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

chore: enable ctrl-c only after changeset is created and executed #5112

Merged

Conversation

KollaAdithya
Copy link
Contributor

@KollaAdithya KollaAdithya commented Jul 21, 2023

Even though this PR looks big, half of the changes are just unit tests.

Better solution than #5097

Old Approach:
I closed PR 5097. In the previous PR #5097, there is a drawback with the logic when users initiated a ctrl-c interruption during the process of proposing infrastructure changes. The issue was that the interruption signal (SIGINT) was being captured even before the changeset is created and executed. As a result, when the user pressed ctrl-c during proposing infrastructure changes, we display a message like Received interrupt for ctrl-c; However, despite the interruption, the code continued to create and execute the changeset, leading to the stack creation process continuing even after the user intended to stop it.

New Approach:
To address the issue, this PR had a better solution for handling ctrl-c interruptions during the process of proposing infrastructure changes. Listening to the SIGINT signal is performed only after the changeset is created and executed. The necessary changes to achieve this have been made by relocating the complete logic for waiting and handling the signal to the deploy/cloudformation package.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the Apache 2.0 License.

@KollaAdithya KollaAdithya requested a review from a team as a code owner July 21, 2023 16:12
@KollaAdithya KollaAdithya requested review from Lou1415926 and removed request for a team July 21, 2023 16:12
@github-actions
Copy link

github-actions bot commented Jul 21, 2023

🍕 Here are the new binary sizes!

Name New size (kiB) size (kiB) Delta (%)
macOS (amd) 51476 51400 +0.15
macOS (arm) 51684 51604 +0.16
linux (amd) 45308 45244 +0.14
linux (arm) 43588 43524 +0.15
windows (amd) 42128 42072 +0.13

@codecov-commenter
Copy link

codecov-commenter commented Jul 21, 2023

Codecov Report

Merging #5112 (afaef34) into mainline (55dba4d) will decrease coverage by 0.08%.
Report is 13 commits behind head on mainline.
The diff coverage is 70.50%.

@@             Coverage Diff              @@
##           mainline    #5112      +/-   ##
============================================
- Coverage     69.73%   69.66%   -0.08%     
============================================
  Files           293      295       +2     
  Lines         43003    43243     +240     
  Branches        285      285              
============================================
+ Hits          29988    30124     +136     
- Misses        11552    11643      +91     
- Partials       1463     1476      +13     
Files Changed Coverage Δ
internal/pkg/cli/job_deploy.go 39.80% <22.22%> (-0.54%) ⬇️
internal/pkg/cli/svc_deploy.go 38.59% <25.00%> (-0.45%) ⬇️
...ternal/pkg/deploy/cloudformation/cloudformation.go 75.96% <78.63%> (+1.65%) ⬆️
internal/pkg/deploy/cloudformation/workload.go 60.97% <100.00%> (ø)

... and 8 files with indirect coverage changes

Copy link
Contributor

@iamhopaul123 iamhopaul123 left a comment

Choose a reason for hiding this comment

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

Awesome! Just to double check: do we need to update the deploy and jobdeploy with the same thing?

@KollaAdithya
Copy link
Contributor Author

KollaAdithya commented Jul 21, 2023

With this PR we are already enabling ctrl-c for both svc deploy and job deploy under the hood ctrl-c will automatically apply to copilot deploy

internal/pkg/cli/deploy/rdws.go Outdated Show resolved Hide resolved
internal/pkg/cli/deploy/worker.go Outdated Show resolved Hide resolved
internal/pkg/cli/env_delete_test.go Show resolved Hide resolved
internal/pkg/deploy/cloudformation/cloudformation.go Outdated Show resolved Hide resolved
internal/pkg/deploy/cloudformation/cloudformation_test.go Outdated Show resolved Hide resolved
internal/pkg/deploy/cloudformation/cloudformation_test.go Outdated Show resolved Hide resolved
dannyrandall
dannyrandall previously approved these changes Jul 26, 2023
Copy link
Contributor

@dannyrandall dannyrandall left a comment

Choose a reason for hiding this comment

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

i love this PR!🚀 just some small nits and one more question 😊

internal/pkg/cli/svc_deploy.go Show resolved Hide resolved
internal/pkg/deploy/cloudformation/cloudformation.go Outdated Show resolved Hide resolved
internal/pkg/deploy/cloudformation/cloudformation.go Outdated Show resolved Hide resolved
internal/pkg/deploy/cloudformation/cloudformation.go Outdated Show resolved Hide resolved
internal/pkg/deploy/cloudformation/cloudformation.go Outdated Show resolved Hide resolved
internal/pkg/deploy/cloudformation/cloudformation.go Outdated Show resolved Hide resolved
internal/pkg/deploy/cloudformation/cloudformation.go Outdated Show resolved Hide resolved
@dannyrandall dannyrandall dismissed their stale review July 26, 2023 17:06

meant to comment

Copy link
Contributor

@dannyrandall dannyrandall left a comment

Choose a reason for hiding this comment

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

great work!

@mergify mergify bot merged commit 5109d75 into aws:mainline Jul 26, 2023
12 checks passed
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.

4 participants