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

Do not fail on valid flag combinations for managed ngs #3930

Merged
merged 2 commits into from Jul 5, 2021

Conversation

Callisto13
Copy link
Contributor

Description

Fixes #3929

Checklist

  • Added tests that cover your change (if possible)
  • Added/modified documentation as required (such as the README.md, or the userdocs directory)
  • Manually tested
  • Made sure the title of the PR is a good description that can go into the release notes
  • (Core team) Added labels for change area (e.g. area/nodegroup) and kind (e.g. kind/improvement)

BONUS POINTS checklist: complete for good vibes and maybe prizes?! 🤯

  • Backfilled missing tests for code in same general area 🎉
  • Refactored something and made the world a better place 🌟

@Callisto13 Callisto13 changed the base branch from main to release-0.56 July 5, 2021 11:57
@Callisto13 Callisto13 enabled auto-merge (squash) July 5, 2021 11:58
Comment on lines 442 to +445
func validateManagedNGFlags(cmd *cobra.Command, managed bool) error {
flagsValidOnlyWithMNG := []string{"spot", "instance-types"}
if managed {
return nil
}
Copy link
Contributor

Choose a reason for hiding this comment

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

why is the func call validateManagedFlags and a bool is passed in to indicate its managed? Is it not called because its validating flags for managed nodes? 🙈

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i think it was named to validate the use of managed flags (not managed nodegroup flags), now it only checks for the use of managed flags on unmanaged ngs

it is confusing because technically it is named accurately, but we use "managed" as a shorthand for "managed nodegroups" so our minds fill in a gap which is not there

Copy link
Contributor

Choose a reason for hiding this comment

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

ohhhh. Gotcha. Since for managed its always if managed; return nil would it make sense to move that logic outside of the func? Might make it less confusing

i think it was named to validate the use of managed flags (not managed nodegroup flags), now it only checks for the use of managed flags on unmanaged ngs

it does have NG in the func name though 😉

Copy link
Contributor Author

Choose a reason for hiding this comment

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

would it make sense to move that logic outside of the func? Might make it less confusing

i did think about that, and decided to do the if thing in one place rather than more than 1, don't really care tho 🤷‍♀️

it does have NG in the func name though

oh shit yeh, what was i reading 🤣

Copy link
Contributor

@aclevername aclevername left a comment

Choose a reason for hiding this comment

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

LGTM, 1 nit

@Callisto13 Callisto13 merged commit 00ba7d2 into eksctl-io:release-0.56 Jul 5, 2021
@aclevername
Copy link
Contributor

oh lol automerge was enabled. smh

@Callisto13
Copy link
Contributor Author

you only have yourself to blame tbh

@aclevername
Copy link
Contributor

totally not the fault of the person who enabled it 🤷

@Callisto13
Copy link
Contributor Author

stop approving unapprovable crap!

@Callisto13 Callisto13 deleted the fix-mng-flags-check branch July 5, 2021 12:28
@aclevername
Copy link
Contributor

it was approvalable, my comments were just a nit

@Callisto13
Copy link
Contributor Author

hmmm i wouldn't have approved it :trollface:

Callisto13 added a commit that referenced this pull request Jul 5, 2021
* Tag 0.56.0-rc.0 release candidate

* Do not fail on valid flag combinations for managed ngs (#3930)

Co-authored-by: Niki <18622989+nikimanoledaki@users.noreply.github.com>
Co-authored-by: Claudia <claudiaberesford@gmail.com>
Callisto13 added a commit that referenced this pull request Jul 6, 2021
* Tag 0.56.0-rc.0 release candidate

* Do not fail on valid flag combinations for managed ngs (#3930)

* Add new note for 56-rc (#3938)

Co-authored-by: Niki <18622989+nikimanoledaki@users.noreply.github.com>
Co-authored-by: Claudia <claudiaberesford@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Regression in 0.55.0: --spot is only valid with managed nodegroups
2 participants