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.

Sign up for GitHub

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

Jump to bottom

[RFC-0002] Flux OCI support for Helm #2597

Merged
merged 5 commits into from
Apr 13, 2022
Merged

[RFC-0002] Flux OCI support for Helm #2597

merged 5 commits into from
Apr 13, 2022

Conversation

stefanprodan
Copy link
Member

@stefanprodan stefanprodan commented Mar 30, 2022 •
edited
Loading

Given that Helm v3.8 supports OCI for package distribution, we should extend the Flux Source API to allow fetching Helm charts from container registries.

This proposal is for adding OCI support with minimal API changes to Flux.

@stefanprodan stefanprodan added the area/rfc Feature request proposals in the RFC format label Mar 30, 2022
@stefanprodan stefanprodan force-pushed the rfc-helm-oci branch 2 times, most recently from 2a078e0 to 44114b5 Compare March 30, 2022 11:39
hiddeco
hiddeco reviewed Mar 30, 2022
View reviewed changes
rfcs/helm-oci/README.md Outdated Show resolved Hide resolved
@pjbgf pjbgf added this to the GA milestone Mar 30, 2022
@stefanprodan stefanprodan mentioned this pull request Mar 31, 2022
Closed
wonderflow
wonderflow approved these changes Mar 31, 2022
View reviewed changes
Copy link

@wonderflow wonderflow left a comment

Choose a reason for hiding this comment

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

LGTM, looking forward to integrating this feature into KubeVela!

rfcs/helm-oci/README.md Outdated Show resolved Hide resolved
hiddeco
hiddeco reviewed Mar 31, 2022
View reviewed changes
rfcs/helm-oci/README.md Outdated Show resolved Hide resolved
rfcs/helm-oci/README.md Outdated Show resolved Hide resolved
@souleb souleb mentioned this pull request Apr 1, 2022
Closed
relu
relu reviewed Apr 1, 2022
View reviewed changes
rfcs/helm-oci/README.md Outdated Show resolved Hide resolved
darkowlzz
darkowlzz reviewed Apr 4, 2022
View reviewed changes
rfcs/helm-oci/README.md Outdated Show resolved Hide resolved
souleb
souleb reviewed Apr 5, 2022
View reviewed changes
rfcs/helm-oci/README.md Outdated Show resolved Hide resolved
rfcs/helm-oci/README.md Outdated Show resolved Hide resolved
RedbackThomson
RedbackThomson reviewed Apr 7, 2022
View reviewed changes
rfcs/helm-oci/README.md Outdated Show resolved Hide resolved
rfcs/helm-oci/README.md Outdated Show resolved Hide resolved
rfcs/helm-oci/README.md Outdated Show resolved Hide resolved
makkes
makkes approved these changes Apr 11, 2022
View reviewed changes
Copy link
Member

@makkes makkes left a comment

Choose a reason for hiding this comment

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

I generally like this RFC, thanks @stefanprodan! I figure a HelmRepository of type OCI wouldn't expose any artifact, right? It would rather server exclusively as a data store for URL and optionally credentials. I think that's absolutely fine, just want to make sure I understand it correctly and that we document that in the RFC.

rfcs/helm-oci/README.md Outdated Show resolved Hide resolved
rfcs/helm-oci/README.md Outdated Show resolved Hide resolved
rfcs/helm-oci/README.md Outdated Show resolved Hide resolved
hiddeco
hiddeco approved these changes Apr 12, 2022
View reviewed changes
Copy link
Member

@hiddeco hiddeco left a comment

Choose a reason for hiding this comment

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

Happy with how this slight shift in the proposal has resulted in a workable solution. LGTM, thanks @stefanprodan 🙇

@makkes
Copy link
Member

makkes commented Apr 12, 2022

btw, I would love to update the current PoC implementation @souleb and I came up with to match what's defined in this RFC.

pjbgf
pjbgf approved these changes Apr 12, 2022
View reviewed changes
Copy link
Member

@pjbgf pjbgf left a comment

Choose a reason for hiding this comment

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

@stefanprodan great stuff, LGTM!

darkowlzz
darkowlzz approved these changes Apr 12, 2022
View reviewed changes
Copy link
Contributor

@darkowlzz darkowlzz left a comment

Choose a reason for hiding this comment

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

LGTM!

relu
relu approved these changes Apr 12, 2022
View reviewed changes
Copy link
Member

@relu relu left a comment

Choose a reason for hiding this comment

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

Looks solid! 🙌

phillebaba
phillebaba approved these changes Apr 12, 2022
View reviewed changes
Copy link
Member

@phillebaba phillebaba left a comment

Choose a reason for hiding this comment

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

Looks good. Just a small NIT, there may be a better value for the default type than Default as it wont really be too descriptive what this means if more types are added in the future.

rashedkvm
rashedkvm approved these changes Apr 12, 2022
View reviewed changes
Copy link
Member

@rashedkvm rashedkvm left a comment

Choose a reason for hiding this comment

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

This proposal looks great. Nicely done @stefanprodan

@stefanprodan stefanprodan mentioned this pull request Apr 12, 2022
Merged
@makkes makkes self-assigned this Apr 12, 2022
@makkes
Copy link
Member

makkes commented Apr 13, 2022

According to our RFC process I stepped in as sponsor for this RFC and given we have approval from at least 2 maintainers there's unanimity to merge this.

@stefanprodan this RFC's assigned number is 2.

@stefanprodan stefanprodan marked this pull request as ready for review April 13, 2022 07:27
@stefanprodan stefanprodan changed the title [RFC] Flux OCI support for Helm [RFC-0002] Flux OCI support for Helm Apr 13, 2022
@stefanprodan stefanprodan merged commit 7b49409 into main Apr 13, 2022
@stefanprodan stefanprodan deleted the rfc-helm-oci branch April 13, 2022 08:01
@stefanprodan
Copy link
Member Author

The implementation of this RFC can be tracked here fluxcd/source-controller#669

scottrigby
scottrigby reviewed Apr 13, 2022
View reviewed changes
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.

Late to the party here. Excited to see this maintainable RFC ❤️

@gfichtenholt gfichtenholt mentioned this pull request Jul 1, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@souleb souleb souleb left review comments

@RedbackThomson RedbackThomson RedbackThomson left review comments

@scottrigby scottrigby scottrigby left review comments

@makkes makkes makkes approved these changes

@rashedkvm rashedkvm rashedkvm approved these changes

@phillebaba phillebaba phillebaba approved these changes

@hiddeco hiddeco hiddeco approved these changes

@pjbgf pjbgf pjbgf approved these changes

@wonderflow wonderflow wonderflow approved these changes

@relu relu relu approved these changes

@darkowlzz darkowlzz darkowlzz approved these changes

Assignees

@makkes makkes

Labels
area/rfc Feature request proposals in the RFC format
Projects
Maintainers' Focus
Status: Done
Milestone
Security
Development

Successfully merging this pull request may close these issues.