Another reduction of describe via reducing the number of list calls #4772
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Skarlso
commented
Feb 10, 2022
| @@ -50,10 +50,6 @@ func (c *OwnedCluster) Upgrade(dryRun bool) error { | |||
| return err | |||
| } | |||
|
|
|||
| if err := c.ctl.RefreshClusterStatus(c.cfg); err != nil { | |||
There was a problem hiding this comment.
This was redundant as New already called this.
Skarlso
requested review from
aclevername,
nikimanoledaki,
cPu1 and
Himangini
|
I don't see how my changes here could affect |
Skarlso
dismissed
aclevername’s stale review
I pushed in some more changes, sorry :/
aclevername
reviewed
Feb 11, 2022
aclevername
approved these changes
Feb 11, 2022
|
Integration tests passed on this Branch. ( I pushed it to eksctl directly too with the current change set.) |
cPu1
approved these changes
Feb 14, 2022
7 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Part 2 Of refactoring GetStackTemplate and DescribeStack
This time, Describe is reduced due to the number of reduced list calls during deletion. It's possible to pass around stacks by adding Stack to the nodeGroupStack and reducing the number of calls by moving around lists which already happened at the top level.
Related to: #4635
Integration Test Run results
The 2 failures appear to be unrelated, but I'll make sure.
Nodegroup tests incoming...
Hmmm.... Node listing works by hand, but this test keeps failing which makes me think either the test is missing something that I need to add, or I'm missing something from somewhere. The code should work fine, this is just listing. Basically
get nodegroupsfails but it makes no sense why.Managed Nodegroup test:
Fixed the failure and rerun the failing part:
This is definitely a problem somewhere ^^. I run it isolation so it didn't create that nodegroup 🤦.
But it's fine, because
upgradeNg(initialAl2Nodegroup)this passed so the other one should have as well.Just going to run the integration test suit on my branch -> https://github.com/weaveworks/eksctl-ci/actions/runs/1830405203