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

cli: Add support for helm --max-history command line flag #36677

Merged
merged 1 commit into from
Jan 24, 2025

Conversation

marcofranssen
Copy link
Contributor

Please ensure your pull request adheres to the following guidelines:

  • For first time contributors, read Submitting a pull request
  • All code is covered by unit and/or runtime tests where feasible.
  • All commits contain a well written commit description including a title,
    description and a Fixes: #XXX line if the commit addresses a particular
    GitHub issue.
  • If your commit description contains a Fixes: <commit-id> tag, then
    please add the commit author[s] as reviewer[s] to this issue.
  • All commits are signed off. See the section Developer’s Certificate of Origin
  • Provide a title or release-note blurb suitable for the release notes.
  • Are you a user of Cilium? Please add yourself to the Users doc
  • Thanks for contributing!

Fixes: N.A.

cli: Add support for helm --max-history command line flag

@marcofranssen marcofranssen requested review from a team as code owners December 17, 2024 19:49
@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Dec 17, 2024
@marcofranssen marcofranssen changed the title cli: Add support for helm --max-history cli: Add support for helm --max-history command line flag Dec 17, 2024
@github-actions github-actions bot added cilium-cli This PR contains changes related with cilium-cli cilium-cli-exclusive This PR only impacts cilium-cli binary kind/community-contribution This was a contribution made by a community member. labels Dec 17, 2024
@marcofranssen marcofranssen force-pushed the support-helm-max-history branch from a6d4bad to aa90241 Compare December 18, 2024 09:23
@gandro gandro added the release-note/minor This PR changes functionality that users may find relevant to operating Cilium. label Dec 18, 2024
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Dec 18, 2024
Copy link
Member

@gandro gandro left a comment

Choose a reason for hiding this comment

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

Thank you! Looks good to me. Introducing a limit on the history by default seems reasonable to me. But let's also ping @cilium/sig-cilium-cli

@gandro
Copy link
Member

gandro commented Dec 18, 2024

/test

@marcofranssen marcofranssen force-pushed the support-helm-max-history branch from aa90241 to 45bacf9 Compare December 18, 2024 12:16
@gandro
Copy link
Member

gandro commented Dec 18, 2024

@marcofranssen Please note that rebasing the branch invalidates the CI results, thus requiring us to restart CI. Unless there are persistent CI flakes that reoccur even after a comitter has restarted CI for you, or there are merge conflicts, there should be no need to rebase your branch.

@gandro
Copy link
Member

gandro commented Dec 18, 2024

/test

@marcofranssen
Copy link
Contributor Author

@gandro ok, good to know. I'm very used to that to keep stuff in sync, but will remember this for other contributions on this project. To not do that if there is no need for the sake of that PR.

@gandro
Copy link
Member

gandro commented Dec 18, 2024

Incidentally the CI failures on the upgrade test look unrelated. Let's wait for them to get fixed before we rebase

@marcofranssen
Copy link
Contributor Author

marcofranssen commented Dec 19, 2024

Pushed the fix for go linting just now. Was missing the license header on the upgrade_test.go.

The other failures seem to be unrelated to the CLI changes. Let me know if there is anything else required from me.

@gandro gandro force-pushed the support-helm-max-history branch from 82da336 to 68b9d0f Compare December 19, 2024 11:38
@gandro
Copy link
Member

gandro commented Dec 19, 2024

Thank you! For now, I don't think anything else is required from you. Let's re-run CI

@gandro
Copy link
Member

gandro commented Dec 19, 2024

/test

@gandro
Copy link
Member

gandro commented Dec 19, 2024

The test failiure on the the integration test workflow seems legitimate:

┏━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┓
┃   PANIC  package: github.com/cilium/cilium/cilium-cli/install • TestMaxHistory   ┃
┗━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┛
panic: runtime error: invalid memory address or nil pointer dereference [recovered] 
	panic: runtime error: invalid memory address or nil pointer dereference             
[signal SIGSEGV: segmentation violation code=0x1 addr=0x118 pc=0x2ea95ef]           
                                                                                    
goroutine 54 [running]:                                                             
testing.tRunner.func1.2({0x324dda0, 0x7597110})                                     
	/opt/hostedtoolcache/go/1.23.4/x64/src/testing/testing.go:1632 +0x230               
testing.tRunner.func1()                                                             
	/opt/hostedtoolcache/go/1.23.4/x64/src/testing/testing.go:1635 +0x35e               
panic({0x324dda0?, 0x7597110?})                                                     
	/opt/hostedtoolcache/go/1.23.4/x64/src/runtime/panic.go:785 +0x132                  
github.com/cilium/cilium/cilium-cli/install.TestMaxHistory(0xc00046b6c0?)           
	/home/runner/work/cilium/cilium/cilium-cli/install/upgrade_test.go:21 +0x8f         
testing.tRunner(0xc00046b6c0, 0x4f1efb0)                                            
	/opt/hostedtoolcache/go/1.23.4/x64/src/testing/testing.go:1690 +0xf4                
created by testing.(*T).Run in goroutine 1                                          
	/opt/hostedtoolcache/go/1.23.4/x64/src/testing/testing.go:1743 +0x390               
FAIL	github.com/cilium/cilium/cilium-cli/install	0.070s                

@marcofranssen marcofranssen force-pushed the support-helm-max-history branch from 68b9d0f to f3cca59 Compare December 20, 2024 08:18
@marcofranssen
Copy link
Contributor Author

Fixed

@julianwiedmann
Copy link
Member

drive-by - please squash the two fix-ups into the main patch :)

By using this flag and same default as the upstream Helm project we can
give users same behavior. Without this people will end up with hundreds
of helm releases on their clusters. Using this option we will limit them
by default to 10 releases and allow users to choose for a longer or
shorter history.

Signed-off-by: Marco Franssen <[email protected]>
@marcofranssen marcofranssen force-pushed the support-helm-max-history branch from f3cca59 to 4d92762 Compare January 20, 2025 17:43
@marcofranssen
Copy link
Contributor Author

drive-by - please squash the two fix-ups into the main patch :)

Done! 🎉

@tklauser
Copy link
Member

/test

@tklauser tklauser enabled auto-merge January 21, 2025 08:08
@maintainer-s-little-helper maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Jan 21, 2025
@marcofranssen
Copy link
Contributor Author

Looks like all checks passed :) are we good to merge?

@tklauser tklauser added this pull request to the merge queue Jan 24, 2025
Merged via the queue into cilium:main with commit f19ffc6 Jan 24, 2025
63 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cilium-cli This PR contains changes related with cilium-cli cilium-cli-exclusive This PR only impacts cilium-cli binary kind/community-contribution This was a contribution made by a community member. ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/minor This PR changes functionality that users may find relevant to operating Cilium.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants