Skip to content
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 force-reboot and drain timeouts to chart config and ds #360

Merged

Conversation

cnmcavoy
Copy link
Contributor

Followup from #341, exposes all of the new cli flags to the helm chart.

@cnmcavoy cnmcavoy force-pushed the cnmcavoy/force-reboot-timeout-helm branch 2 times, most recently from a56fb06 to 4ef4c8e Compare April 19, 2021 19:27
@anthr76
Copy link

anthr76 commented Apr 24, 2021

Would it be possible to add reboot command as well or is another PR warranted?

@evrardjp
Copy link
Collaborator

@anthr76 the reboot command is already flexible, in our main branch. We are just waiting for the next kubernetes kind nodes to release our recent work!

@@ -96,7 +96,7 @@ func main() {
Run: root}

rootCmd.PersistentFlags().BoolVar(&forceReboot, "force-reboot", false,
"force a reboot even if the drain is still running (default: false)")
"force a reboot even if the drain fails or times out (default: false)")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if you would like this clarification to be included in the next release (1.7.0), it's probably better to add it into another PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not overly concerned, I just noticed it was slightly incorrect when doing the helm chart changes and wanted to correct it.

@bstrdsmkr
Copy link

@anthr76 the reboot command is already flexible, in our main branch. We are just waiting for the next kubernetes kind nodes to release our recent work!

I'm not sure I understand your response here -- I believe @anthr76 is asking to have the --reboot-command included as an explicit option in the chart, even though it (as are these) is already settable via extraArgs. I didn't see an explicit option in the chart in the main branch. Did I just miss it?

@anthr76
Copy link

anthr76 commented May 10, 2021

@bstrdsmkr Is indeed correct.. Didn't see that extraArgs bit before so not totally necessary. Just a nice to have at this point.

@evrardjp
Copy link
Collaborator

You're right, it seems like this slipped through the cracks. PRs welcome!

@anthr76
Copy link

anthr76 commented May 11, 2021

I'll be happy to. Any idea on when this gets merged along with the new release?

@bstrdsmkr
Copy link

as a sanity check, is there a generic way to convert between the flag name and a yaml safe name? That way you could generate them from the yaml values instead of hard coding each one

@evrardjp
Copy link
Collaborator

as a sanity check, is there a generic way to convert between the flag name and a yaml safe name? That way you could generate them from the yaml values instead of hard coding each one

That would be amazing, but I am not sure we have that. @ckotzbauer what do you think of the idea? We could maybe write a utility function that does that in the helm chart, and leverage it? Or is there any other way?

@evrardjp
Copy link
Collaborator

@cnmcavoy this will need a rebase, can you tackle it? That would be lovely.

@ckotzbauer
Copy link
Member

@evrardjp @bstrdsmkr I think the "kebabcase" function could help us: https://helm.sh/docs/chart_template_guide/function_list/#kebabcase

@cnmcavoy cnmcavoy force-pushed the cnmcavoy/force-reboot-timeout-helm branch from 4ef4c8e to 644e892 Compare June 22, 2021 15:21
@EduardFazliev
Copy link

Is there any chance that this will be merged some day? Is there a way to help to speed up the process?

@ckotzbauer
Copy link
Member

ckotzbauer commented Sep 15, 2021

@cnmcavoy Can you rebase this again please?
I think this should be ready to merge in general, if there are no other objections. I would be ok to include the clarification in main.go
/cc @evrardjp

@cnmcavoy cnmcavoy force-pushed the cnmcavoy/force-reboot-timeout-helm branch from 644e892 to cee15cf Compare September 15, 2021 15:42
@cnmcavoy
Copy link
Contributor Author

rebased.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants