-
Notifications
You must be signed in to change notification settings - Fork 202
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 node labelling #541
Add node labelling #541
Conversation
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.
Hi @liger1978,
thanks for your PR and working on the issue. This is much appreciated!
Some thoughts to the label-feature:
- The labels are only added, never removed. The zalando-usecase is toggling the node-label-value. Would it be better to remove the
before-labels
when theafter-labels
are applied? I think this is more in line with user expectations. - I'm not sure about the cli-flag-names. What about
pre-reboot-node-labels
andpost-reboot-node-labels
?
Can you please separate the spelling corrections and readme updates from this PR? It's hard to review, when there are many other changes which aren't directly related to a PR. Please always open separate PRs for that.
Thanks!
/cc @evrardjp @jackfrancis Thoughts?
@ckotzbauer I'm not sure what "removing a label" means in this context. In ready state, nodes will already have a label like |
@ckotzbauer I've updated the PR with the other requested changes. |
Thanks for your changes. |
@ckotzbauer OK, so what would you prefer? Please can you be specific in the behaviour you require as I am struggling to understand what you want (my limitation no doubt). |
I think the simplest thing would be a validation, that all label-keys set as pre-reboot-label are also set as post-reboot-labels and vice versa. This should also be described at the README. |
@ckotzbauer I've added the verification and updated the readme as requested. |
@jackfrancis I have implemented your suggestions. |
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.
From my perspective this looks good now. @jackfrancis do you want to have a final look?
I would include this into 1.10.0 if there are not objections.
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.
lgtm
Thanks. Please can it be merged! |
@liger1978 One minor niggle. The current implementation has pre-reboot-node-labels and after-reboot-node-labels, which I find confusing. IMO the latter should be post-reboot-node-labels as per the original suggestion from @ckotzbauer. Or alternatively before-reboot-node-labels and after-reboot-node-labels. |
Yes, you are right @erwbgy. Thanks for the hint. This should be fixed. |
@erwbgy @ckotzbauer @jackfrancis I have made this change. Please review/approve/merge. Thank you! |
@ckotzbauer Thank you very much for merging! Do you have any idea when we can expect a new release with these changes included? |
I think in the next week. |
Fixes #509