pkg/cmd: clarify install options, document them #31413
Conversation
pull-request-size
bot
added
the
size/S
|
@TerryHowe does your versioning scheme apply to the library as well as the flags for users? Also, isn't the point of alpha releases that you can still break APIs? 😅 |
|
My preference would be to have a broader refactor, if I'm honest - the flow that I triggered by setting |
|
The beta is out for v4 and there aren't supposed to be any breaking changes with the CLI or SDK at this point. |
|
@mattfarina hi! I found a set of flags on the install SDK which cause Helm v4 to silently delete CRDs and cause catastrophic data loss on clusters. Do you think this warrants some breaking changes in the SDK (but not the CLI) to make it less likely that users hit this? @TerryHowe I appreciate what you are saying but I don't think you are commenting on the important part of my question - should Helm silently delete CRDs on a cluster? Data loss on upgrade seems like the cardinal sin here and making that hard, if not impossible, seems like a good goal. (And a breaking change in the SDK, even if in a beta, seems like a much, much, much smaller issue.) |
|
Documentation would help the poorly named booleans and would not break SDK users. Maybe put a TODO in there to update for v5. That would be my guess on where this is going to have to go. |
Without a careful reading of the installation code, I made a number of mistakes configuring the install client with the `IncludeCRDs` and `SkipCRDs` options. The new names proposed in this commit aim to clarify the intent of these options and make such mistakes impossible in the future. Signed-off-by: Steve Kuznetsov <stekuznetsov@microsoft.com>
stevekuznetsov
force-pushed
the
skuznets/better-doc
branch
from
ec3ace9 to
bad180f
Compare
|
Sure, that's the least we can do. |
There was a problem hiding this comment.
Pull Request Overview
This PR improves documentation for CRD-related installation options in Helm's Install action by adding detailed comments explaining the purpose and implications of SkipCRDs and IncludeCRDs flags.
Key changes:
- Added documentation for
SkipCRDsexplaining it prevents CRD operations during chart installation - Added documentation for
IncludeCRDswarning about the risks of including CRDs in release manifests during live cluster installations
| // RollbackOnFailure enables rolling back (uninstalling) the release on failure if set | ||
| RollbackOnFailure bool | ||
| RollbackOnFailure bool | ||
| // SkipCRDs opts the chart installation out of installing or upgrading CRs on the cluster. By |
There was a problem hiding this comment.
Corrected abbreviation from 'CRs' to 'CRDs' to match the context about Custom Resource Definitions.
| // SkipCRDs opts the chart installation out of installing or upgrading CRs on the cluster. By | |
| // SkipCRDs opts the chart installation out of installing or upgrading CRDs on the cluster. By |
Copilot uses AI. Check for mistakes.
| // SkipCRDs opts the chart installation out of installing or upgrading CRs on the cluster. By | ||
| // default, CRDs are installed (or upgraded) during a chart installation and AlreadyExists errors | ||
| // are ignored. When this flag is set, no operations over CRDs will be made against the cluster | ||
| // during installation. |
There was a problem hiding this comment.
| // SkipCRDs opts the chart installation out of installing or upgrading CRs on the cluster. By | |
| // default, CRDs are installed (or upgraded) during a chart installation and AlreadyExists errors | |
| // are ignored. When this flag is set, no operations over CRDs will be made against the cluster | |
| // during installation. | |
| // SkipCRDs If set to false, CRDs are installed (or upgraded) during a chart installation and | |
| // AlreadyExists errors are ignored. If set to true, no operations on CRDs are performed on | |
| // the cluster during installation. |
2 participants
Without a careful reading of the installation code, I made a number of mistakes configuring the install client with the
IncludeCRDsandSkipCRDsoptions. The new names proposed in this commit aim to clarify the intent of these options and make such mistakes impossible in the future.