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

Suspend processes #132

Merged
merged 17 commits into from
Jun 7, 2020
Merged

Conversation

SaiVishwas
Copy link
Contributor

@SaiVishwas SaiVishwas commented Jun 2, 2020

Fixes #119

Assumption: In the desired/target state, only the processes mentioned in the input yaml need to be suspended on the ASG
Scenarios tested:
During Create

  1. When the suspendProcesses config is not present, then no action taken
  2. When the config is passed, post ASG creation, the processes mentioned are suspended using a single AWS API call

During Update

  1. When the target list mentioned in yaml is different from current list of suspended processes in ASG, only then it is updated (resumed/suspended)

@eytan-avisror please review and let me know if you would like me to improve anything.
Thank you

@SaiVishwas SaiVishwas requested a review from a team as a code owner June 2, 2020 15:40
@CLAassistant
Copy link

CLAassistant commented Jun 2, 2020

CLA assistant check
All committers have signed the CLA.

@codecov
Copy link

codecov bot commented Jun 2, 2020

Codecov Report

Merging #132 into master will decrease coverage by 0.43%.
The diff coverage is 66.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #132      +/-   ##
==========================================
- Coverage   88.93%   88.49%   -0.44%     
==========================================
  Files          10       10              
  Lines        1193     1217      +24     
==========================================
+ Hits         1061     1077      +16     
- Misses         91       95       +4     
- Partials       41       45       +4     
Impacted Files Coverage Δ
controllers/provisioners/eks/create.go 92.85% <0.00%> (-2.27%) ⬇️
controllers/provisioners/eks/update.go 96.58% <0.00%> (-0.84%) ⬇️
controllers/provisioners/eks/helpers.go 87.46% <80.00%> (-0.47%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5e020b3...6a1fe67. Read the comment docs.

@eytan-avisror
Copy link
Collaborator

Thanks for the PR and contribution @SaiVishwas
Some minor comments added.

@eytan-avisror
Copy link
Collaborator

Thanks for the comments you addressed @SaiVishwas .. looks great!
Added few last things we need to change.
Also, I noticed there is a coverage drop, can you run make coverage and check? or look in the codecov report and see if you can add a test?
It's not a significant drop, but might be good to check if it's something we want to cover

@SaiVishwas SaiVishwas requested a review from a team as a code owner June 4, 2020 07:05
@SaiVishwas
Copy link
Contributor Author

Hi @eytan-avisror ,
I have made the suggested changes. The newly added methods are being tested as part of the unit tests. I went through the code coverage report and drop in coverage is due to the

err != nil { return err }

code block.

Thank you

@eytan-avisror
Copy link
Collaborator

Thanks @SaiVishwas
We should be able to merge this shortly, had one more question above.

Copy link
Collaborator

@eytan-avisror eytan-avisror left a comment

Choose a reason for hiding this comment

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

Amazing work @SaiVishwas 🥇

@eytan-avisror
Copy link
Collaborator

15 scenarios (15 passed)
72 steps (72 passed)
22m41.99080058s
testing: warning: no tests to run
PASS
ok github.com/keikoproj/instance-manager/test-bdd 1362.315s [no tests to run]

@eytan-avisror eytan-avisror merged commit 5e68ce0 into keikoproj:master Jun 7, 2020
eytan-avisror added a commit that referenced this pull request Jun 7, 2020
PR #132 was missing the generated deepcopy changes.
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.

Support to suspend certain autoscaling processes on ASG
3 participants