-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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鈥檒l occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix setting ownership status when listing clusters #4911
Conversation
Good job getting a fix quickly! :) Could you add unit tests please? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we get some unit tests if possible (oops didn't see niki comments), and update the unowned cluster integration test?
01209e6
to
d340dd5
Compare
|
||
// AssertContainsCluster asserts that the output of the specified command contains the specified cluster | ||
func AssertContainsCluster(cmd runner.Cmd, out GetClusterOutput) { | ||
Expect(cmd).To(runner.RunSuccessfullyWithOutputStringLines( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Intentionally not using a dot import because runner.Cmd is also being used here.
Instead of using the `clusterName` argument, the method was obtaining the cluster name via the `spec` field.
d340dd5
to
e5e25d0
Compare
PR eksctl-io#4911 fixed a bug where the `clusterName` parameter was not being used when obtaining a cluster's ownership for `get clusters`. The logic for checking owned clusters, however, relied on this behaviour, breaking the check for cluster ownership.
* Fix checking cluster stack for owned clusters PR #4911 fixed a bug where the `clusterName` parameter was not being used when obtaining a cluster's ownership for `get clusters`. The logic for checking owned clusters, however, relied on this behaviour, breaking the check for cluster ownership. * Added unit tests * Apply suggestions from code review Co-authored-by: Jake Klein <jakelarsj@gmail.com> * Added error case. * Fix error import Co-authored-by: Gergely Brautigam <182850+Skarlso@users.noreply.github.com> Co-authored-by: Jake Klein <jakelarsj@gmail.com>
Description
Instead of using the
clusterName
argument, the method was obtaining the cluster name via thespec
field.Fixes #4888
This changelist correctly sets the
EKSCTL CREATED
column toTrue
for clusters created via eksctl:Checklist
README.md
, or theuserdocs
directory)area/nodegroup
) and kind (e.g.kind/improvement
)BONUS POINTS checklist: complete for good vibes and maybe prizes?! 馃く