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.
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
add enable-feature command #44
add enable-feature command #44
Changes from all commits
dcac8f3
378b39c
b33810a
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Check warning on line 29 in pkg/cmd/enable-feature.go
pkg/cmd/enable-feature.go#L26-L29
Check warning on line 37 in pkg/cmd/enable-feature.go
pkg/cmd/enable-feature.go#L37
Check warning on line 41 in pkg/cmd/enable-feature.go
pkg/cmd/enable-feature.go#L41
Check warning on line 78 in pkg/cmd/enable-feature.go
pkg/cmd/enable-feature.go#L78
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.
I think we can always update the annotations, the result shouldn't change
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.
I would keep it as is. Just to make sure we do not touch the annotation map at all if the initial feature list is not set. There is still a difference between an empty map and nil.
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.
yes, as Alexey mentioned, it's not the same. We need to make sure that the logic will work when annotations are no initialized (the map is nil). If we don't handle such a case properly, then we could end up with panic trying to modify a map that is nil.
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.
I see, but applying my suggestion the tests are still passing, so I'm wondering if we are missing some specific case that sets the annotation to nil to cover this?
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.
It's the other way around. Your version puts lower requirements on the code than the current test.
Imagine that we don't have these 3 lines of the code
ksctl/pkg/cmd/enable-feature.go
Lines 95 to 97 in b33810a
Your version of the tests will pass, but the current version will fail
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.
Ok I see now, as you explained in our 1:1 this check makes so that we test the nil scenario by not setting any annotation in the setup part of the test and leaving it as nil in the first iterartion. Thanks for explaining and sorry fo the confusion.
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.
same here, we could always update the annotations I believe
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.
The same - we need to verify the case when there is no annotation provided at all (the map is nil).
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.
same here, when I try this change the tests are still passing