-
Notifications
You must be signed in to change notification settings - Fork 49
Feat: Added functionalities to remove duplicate parameters and overwrite existing ones. #506
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
Conversation
…gement, and added functionalities to remove duplicate parameters and overwrite existing ones.
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.
Thanks Danny!
I think if this function works well, it could be good to expand to using this functionality in other commands in XPK in subsequent PRs. But that should be decided by XPK folks.
And I'd suggest adding some more context to the description that this is necessary for any default flags that are overridden by the user (e.g. the networking flags and such).
Description should be better. Is this change only for extra args passed by string? |
@pawloch00 Please review it again. Thanks! |
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.
This looks mostly good to me, but still a few lingering questions.
…Also, made minor code fixes.
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 things look largely good minus the few leftover comments. Thanks!
@SujeethJinesh Maybe the xpk owner needs to verify? |
Requires xpk owner verification. @pawloch00 or @Obliviour |
Please merge develop and fix pipeline |
@DannyLiCom you have been invited as collaborator. Please switch to xpk branch instead of fork and recreate your PR. Sorry for the inconvenience |
I think its duplicate of #527, closing @DannyLiCom |
Fixes / Features
Testing / Documentation
Can be tested when creating a cluster using XPK.