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

Allow the Helm timeout to be set on uninstall #1550

Merged
merged 1 commit into from
Apr 27, 2023

Conversation

meyskens
Copy link
Member

@meyskens meyskens commented Apr 27, 2023

When running Helm in the CI setup i came against the limit that a cleanup of the tests namespace took longer than the Helm default of 5 minutes. However the timeout option was not exposed in out options thus could not be changed.

This PR exposes the --timeout helm option on uninstall when running in Helm mode.

Signed-off-by: Maartje Eyskens <maartje.eyskens@isovalent.com>
@meyskens meyskens requested a review from a team as a code owner April 27, 2023 13:29
@meyskens meyskens requested a review from asauber April 27, 2023 13:29
@meyskens meyskens temporarily deployed to ci April 27, 2023 13:29 — with GitHub Actions Inactive
@@ -325,6 +325,8 @@ func newCmdUninstallWithHelm() *cobra.Command {
}

addCommonUninstallFlags(cmd, &params)

cmd.Flags().DurationVar(&params.Timeout, "timeout", defaults.UninstallTimeout, "Maximum time to wait resources to be deleted")
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
cmd.Flags().DurationVar(&params.Timeout, "timeout", defaults.UninstallTimeout, "Maximum time to wait resources to be deleted")
cmd.Flags().DurationVar(&params.Timeout, "timeout", defaults.UninstallTimeout, "Maximum time to wait for resources to be deleted")

@@ -325,6 +325,8 @@ func newCmdUninstallWithHelm() *cobra.Command {
}

addCommonUninstallFlags(cmd, &params)

Copy link
Member

Choose a reason for hiding this comment

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

nit: for consistency with the rest of the file, there shouldn't be a newline here

@maintainer-s-little-helper maintainer-s-little-helper bot added ready-to-merge This PR has passed all tests and received consensus from code owners to merge. labels Apr 27, 2023
@michi-covalent michi-covalent merged commit 90431a1 into cilium:main Apr 27, 2023
12 checks passed
meyskens added a commit to meyskens/cilium-cli that referenced this pull request Apr 28, 2023
This PR got merged with small nit comments.
This adressed those in a seperate commit.

Signed-off-by: Maartje Eyskens <maartje.eyskens@isovalent.com>
meyskens added a commit to meyskens/cilium-cli that referenced this pull request Apr 28, 2023
The PR cilium#1550 got merged with small nit comments.
This adressed those in a seperate commit.

Signed-off-by: Maartje Eyskens <maartje.eyskens@isovalent.com>
tklauser pushed a commit that referenced this pull request Apr 28, 2023
The PR #1550 got merged with small nit comments.
This adressed those in a seperate commit.

Signed-off-by: Maartje Eyskens <maartje.eyskens@isovalent.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-to-merge This PR has passed all tests and received consensus from code owners to merge.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants