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 support for creating helm packages without needing the helm binary #72

Merged
merged 4 commits into from
Aug 27, 2020

Conversation

paulczar
Copy link
Collaborator

@paulczar paulczar commented Aug 5, 2020

Signed-off-by: Paul Czarkowski username.taken@gmail.com

Signed-off-by: Paul Czarkowski <username.taken@gmail.com>
@helm-bot helm-bot added the size/L label Aug 5, 2020
Copy link
Member

@unguiculus unguiculus left a comment

Choose a reason for hiding this comment

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

I like the idea of adding chart packaging. 👍

Code for indexing and uploading is in the releaser package. The Cobra commands delegate to that code. Here, you implement everything within the command. That's inconsistent. I'd suggest you match the existing style. I'd be nice to have tests as well.

cr/cmd/package.go Outdated Show resolved Hide resolved
cr/cmd/package.go Outdated Show resolved Hide resolved
cr/cmd/package.go Outdated Show resolved Hide resolved
Signed-off-by: Paul Czarkowski <username.taken@gmail.com>
@helm-bot helm-bot added size/XL and removed size/L labels Aug 6, 2020
Signed-off-by: Reinhard Nägele <unguiculus@gmail.com>
Copy link
Member

@unguiculus unguiculus left a comment

Choose a reason for hiding this comment

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

The upload command has a flag for specifying the target commit for the release. I think we should have the same flag here and check out this commit before packaging.

See:

@paulczar
Copy link
Collaborator Author

@unguiculus I'm not sure I see the benefit? A user wanting to package a previous release based on a commit can check out that commit, do the package, then check out main again. I feel like adding the need to run git checkout twice in the code is potentially fragile.

Copy link
Member

@unguiculus unguiculus left a comment

Choose a reason for hiding this comment

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

Yeah, maybe it's better to keep things simple here.

Copy link
Member

@davidkarlsen davidkarlsen left a comment

Choose a reason for hiding this comment

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

Just a nit 🤷 on docs - else lgtm 🎉

@paulczar
Copy link
Collaborator Author

oh no @davidkarlsen your suggested change that I just hit "accept suggestion" didn't have a DCO 💀

Co-authored-by: David J. M. Karlsen <david@davidkarlsen.com>
Signed-off-by: Paul Czarkowski <username.taken@gmail.com>
@paulczar
Copy link
Collaborator Author

Are we good to move forward with this? Do we need another reviewer to approve ? @scottrigby

@scottrigby
Copy link
Member

Hey @paulczar i'm going to try to look over this today

Comment on lines +32 to +33
If you wish to use advanced packaging options such as creating signed
packages or updating chart dependencies please use "helm package" instead.`,
Copy link
Member

Choose a reason for hiding this comment

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

Looks like we could pass any helm package options the way you wrote CreatePackages()?

Copy link
Member

@scottrigby scottrigby Aug 26, 2020

Choose a reason for hiding this comment

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

Not suggesting we need to add that in this PR, but wondering if we could make it easy to encourage users to automate signing their packages. Would love to see that happen more.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Perhaps, but I didn't have the time to test a bunch of that stuff, so I thought for the first PR we should just focus on the most basic use case.

Copy link
Member

Choose a reason for hiding this comment

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

SGTM

Copy link
Member

@scottrigby scottrigby left a comment

Choose a reason for hiding this comment

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

Package options question can be a follow-up 👍

@unguiculus unguiculus merged commit be24fd2 into helm:master Aug 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants