-
Notifications
You must be signed in to change notification settings - Fork 429
Added ExtraArgs to kine #5871
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
base: main
Are you sure you want to change the base?
Added ExtraArgs to kine #5871
Conversation
Hi @psaradhi, First of all, thanks for the pull request. I haven't had a proper look at the PR yet but the couple checks failing: 1- The DCO check verifies that all your commits are signed off. To sign off the commits the commit must include 2- The linter fails because you didn't include in the commit the autogenerated code. if you run Also I see you closed the first pull request and opened a second one. You don't need to do this to make modifications, you can modify your local branch and push it to github again. You can also use Finally, this change would require a documentation update. You can check previous similar PRs as a reference: |
The DCO error is legit, (you have to squash and sign off your commits). Don't worry about the etcdmember smoketest (it's flaky and unrelated to you) and the Go lint/Validate OS tests is due to a problem github had this morning. We need to discuss it internally but other than the DCO I think this is OK |
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 probably needs a small change to the docs in configuration.md, too.
aaa0d34
to
da1933e
Compare
Please review |
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.
@psaradhi Thanks. I think we need to revisit the switch statement, though.
modified static clusterconfigs.yaml to update the sample config to include extrArgs for Kine Modified Kine extraArgs description to pass linter check Ignoring override values for endpoint, listen-address, metrics-bind-address Signed-off-by: psaradhi <[email protected]>
da1933e
to
79a2a25
Compare
You are absolutely right about the switch and issue continue. I removed it. |
@psaradhi I created a PR, but then closed it in favour of yours. I added the extraArgs parameter to the TestKinePartialConfigLoading test and noticed that the DataSource doesn't take the default value. You can see my fix here: cgroschupp@8c9f80c |
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 @psaradhi! Would you mind raising another PR with your fix, @cgroschupp, or would you be okay with me cherry-picking it myself?
@twz123 it is better to add some tests to this PR. |
This pull request has merge conflicts that need to be resolved. |
@twz123 - apologies for not being active for the last few days. I think @cgroschupp had the tests in the PR.. Should I add the same tests ? or will you cherry pick those tests ? what is expected from me to complete this PR ? |
The PR checklist is clear on this point
@psaradhi please add the tests to prove your feature. |
@psaradhi Yes, please cherry-pick the tests here. Also, this PR needs a rebase, since it has a merge conflict. |
Added ExtraArgs to kine
Modified static clusterconfigs.yaml to update the sample config to include extrArgs for Kine
Description
Fixes #5182
Type of change
How Has This Been Tested?
Checklist