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

HIP-0011: Informational Helm CRD HIP #138

Merged
merged 4 commits into from
Apr 19, 2021
Merged

Conversation

technosophos
Copy link
Member

This is a draft of a document to provide a complete explanation of the CRD conundrum in Helm. The intention is to have one formal statement that we can direct others to so that they understand:

  • The nature of the problem
  • The reason for Helm's current implementation
  • The guidelines for future alternatives
  • The time frame for reconsidering our design choices

Signed-off-by: Matt Butcher matt.butcher@microsoft.com

@technosophos technosophos self-assigned this Aug 7, 2020
@helm-bot helm-bot added the size/L label Aug 7, 2020
Copy link
Contributor

@mattfarina mattfarina left a comment

Choose a reason for hiding this comment

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

In general, I like this write-up. I think every maintainer and contributor to the space should read it.

architecture/crds.md Outdated Show resolved Hide resolved
architecture/crds.md Outdated Show resolved Hide resolved
architecture/crds.md Outdated Show resolved Hide resolved
Copy link

@thomastaylor312 thomastaylor312 left a comment

Choose a reason for hiding this comment

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

💯


The next two subsections explain these.

### Shared Global Resources

Choose a reason for hiding this comment

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

Nit: remove extra space between Global and Resources

architecture/crds.md Outdated Show resolved Hide resolved
architecture/crds.md Outdated Show resolved Hide resolved
architecture/crds.md Outdated Show resolved Hide resolved
architecture/crds.md Outdated Show resolved Hide resolved
architecture/crds.md Outdated Show resolved Hide resolved
architecture/crds.md Outdated Show resolved Hide resolved
architecture/crds.md Outdated Show resolved Hide resolved
architecture/crds.md Outdated Show resolved Hide resolved
Copy link
Contributor

@hickeyma hickeyma left a comment

Choose a reason for hiding this comment

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

This is a really good detailed discussion on an area which has proved difficult to explain in the Helm community. It will be helpful when trying to find solutions to people's issues. Thanks @technosophos

architecture/crds.md Outdated Show resolved Hide resolved
Copy link
Member

@bacongobbler bacongobbler left a comment

Choose a reason for hiding this comment

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

Hi @technosophos!

As #140 has now been approved and accepted, would you mind following the process described in HIP 1 and rewrite this as an "informational" HIP?

Let me know if you have any questions about the process. I'd be happy to answer them.

Thank you!

@jtigger
Copy link

jtigger commented Oct 11, 2020

This is a challenging situation. Not just in where all the various machinery are at, but where we are as a result as a broader community.

That y'all took the time to elaborate all of this accounts to genuine "thought leadership" within this community, in my book.

As a member, I'm grateful. 🙏

@elmariofredo
Copy link

Hi,

I have run into issue of helm installing and not updating CRDs and end up here. I have tried to read this explanation deeply and
I have two questions:

  1. Can you please provide examples what exactly could go wrong and by examples I mean real world examples not just statement that this can happen as I feel that either I'm living in strange bubble or helm contributors see more into how chart authors are building their charts. If there is small number of rare poorly designed charts is it really worth of these precautions? I think that It's helm chart authors responsibility to build quality chart which will not break users setup not helm same way that developer is responsible for their program and not author of programming language which developer used.

  2. If helm really don't want to handle CRD then do not handle them at all not even installing as it doesn't make sense and can lead to erratic behaviour like upgrade old crd and new controller.

Sorry to jump directly to PR but as you have closed related bugs there is no other space to ask my question.

Thank You

architecture/crds.md Outdated Show resolved Hide resolved
architecture/crds.md Outdated Show resolved Hide resolved
architecture/crds.md Outdated Show resolved Hide resolved
architecture/crds.md Outdated Show resolved Hide resolved
Copy link

@philoserf philoserf left a comment

Choose a reason for hiding this comment

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

Adding some suggestions on word use, phrasing.

architecture/crds.md Outdated Show resolved Hide resolved
architecture/crds.md Outdated Show resolved Hide resolved
architecture/crds.md Outdated Show resolved Hide resolved
architecture/crds.md Outdated Show resolved Hide resolved
architecture/crds.md Outdated Show resolved Hide resolved
architecture/crds.md Outdated Show resolved Hide resolved
@technosophos
Copy link
Member Author

I was granted HIP approval, but no HIP number was granted. So I am claiming HIP-0011.

@technosophos technosophos changed the title Initial add of Helm CRD document HIP-0011: Informational Helm CRD HIP Mar 26, 2021
Signed-off-by: Matt Butcher <matt.butcher@microsoft.com>
Signed-off-by: Matt Butcher <matt.butcher@microsoft.com>
Signed-off-by: Matt Butcher <matt.butcher@microsoft.com>
@technosophos
Copy link
Member Author

I have reformatted and fixed up this document. Anything I marked as "resolved" I either changed or decided not to further consider the matter. Given that I can't solve the problem, I believe that this HIP is suitable for merging as an informational document. From there, perhaps we can figure out a better way to have the ongoing dialog about this issue--and have a stable reference that others can use.

hips/hip-0011.md Outdated
authors: [ "Matt Butcher <matt.butcher@microsoft.com>" ]
created: "2021-03-26"
type: "informational"
status: "draft"
Copy link
Member

Choose a reason for hiding this comment

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

Typically "informational" HIPs are considered final after approval, as they're meant to serve as information to the general Helm community, and it's not really considered a living document compared to "feature" or "process" HIPs. Do you expect there to be further changes to this document or would you consider this as a final draft?

Do keep in mind that newer HIPs can replace or deprecate older HIPs, so if we change how CRDs are handled later on we can mark this one as superseded.

Copy link
Member Author

Choose a reason for hiding this comment

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

The intention was to document why Helm 3 works the way it does. So it is informational IMO. If a new thing happens for Helm 4, I envision a new HIP for that.

The main thing I was hoping to accomplish is to give people a substantial reference to understand what we have already done and why.

Copy link
Contributor

Choose a reason for hiding this comment

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

@bacongobbler @technosophos I think the status can be set to final. Is that the only change being noted here?

Copy link
Member

Choose a reason for hiding this comment

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

yes

Copy link
Member Author

Choose a reason for hiding this comment

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

@bacongobbler are we good to merge at this point? You still have a nay vote on this PR.

Signed-off-by: Matt Butcher <matt.butcher@microsoft.com>
@technosophos technosophos merged commit 16c18cf into helm:main Apr 19, 2021
@technosophos technosophos deleted the doc/crds branch April 19, 2021 19:47
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.

None yet