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

fix: honor defaults from drain dialog in request #2239

Merged
merged 3 commits into from
Oct 12, 2023

Conversation

fgalind1
Copy link
Contributor

@fgalind1 fgalind1 commented Oct 4, 2023

Describe the bug

Draining a node doesn't honor default values shown in the dialog. In particular drain happens with grace period of 0s instead of the default of -1

How to Reproduce

When draining a node, we noticed that when you do not modify the default gracePeriod of -1 from the UI, based on the API logs it is sending gracePeriod: 0, which it's pretty similar to a force non-graceful termination which doesn't let the termination flow go.

image

If we update the default to a different value, that other value is the one that is sent as expected

If you delete the default gracePeriod of -1 from the dialog, and type again -1, this time it works and do not sends a gracePeriod, so I wonder if our issue is related to this same issue

image

Expected behavior
Drain operation should honor default values shown in the dialog

Possible related to issue #2129

@fgalind1 fgalind1 changed the title fix: honor defaults from dialog into request fix: honor defaults from drain dialog in request Oct 4, 2023
Copy link
Owner

@derailed derailed left a comment

Choose a reason for hiding this comment

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

@fgalind1 Good catch! Thanks for the PR!!

internal/view/drain_dialog.go Outdated Show resolved Hide resolved
Copy link
Owner

@derailed derailed left a comment

Choose a reason for hiding this comment

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

@fgalind1 Thank you for the updates Felipe!
Looks like you need to update the remaining refs to default

@fgalind1
Copy link
Contributor Author

@derailed would you mind helping approving the workflow again pls?

@derailed derailed merged commit e906fa6 into derailed:master Oct 12, 2023
3 checks passed
@derailed derailed mentioned this pull request Nov 8, 2023
placintaalexandru pushed a commit to placintaalexandru/k9s that referenced this pull request Apr 3, 2024
* fix: honor defaults from dialog into request

* reuse options/defaults from arg

* fix additional references for defaults
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants