Add opt-in --crd flag to install and uninstall commands#470
Add opt-in --crd flag to install and uninstall commands#470kelos-bot[bot] wants to merge 2 commits intomainfrom
Conversation
CRDs define custom resource types and deleting them cascades to all custom resources in the cluster. Add a --crd flag (default true) to both install and uninstall commands so users can explicitly control whether CRDs are installed or removed. When --crd=false is passed to install, CRD application is skipped. When --crd=false is passed to uninstall, CRD deletion is skipped, keeping custom resources intact. Closes #461 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
c7d8cff to
5fd6d7c
Compare
gjkim42
left a comment
There was a problem hiding this comment.
Is it better to just install crds if it's provided?
|
/reset-worker |
CRDs should be treated carefully since deleting them cascades to all custom resources in the cluster. Change --crd from default true to default false so users must explicitly opt in with --crd to install or remove CRDs. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
2 issues found across 3 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="internal/cli/install.go">
<violation number="1" location="internal/cli/install.go:96">
P0: The `--crd` flag default is changed from `true` to `false`, which is a breaking change that contradicts the PR description (which says default is `true`) and the README Quick Start guide (which says `kelos install` installs CRDs). Users running `kelos install` will silently skip CRD installation. If the intent is truly `default=false`, the README must be updated; if not, this default should be `true`.
(Based on your team's feedback about keeping documentation and config updated with changes.) [FEEDBACK_USED]</violation>
<violation number="2" location="internal/cli/install.go:183">
P2: The `--crd` flag default is changed from `true` to `false` for uninstall, contradicting the PR description which says default is `true`. While `default=false` is arguably the safer choice for uninstall (since deleting CRDs cascades to all custom resources), this contradicts the stated design. If `default=false` is intentional, the PR description needs correction; if not, restore `true`.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| cmd.Flags().BoolVar(&dryRun, "dry-run", false, "print the manifests that would be applied without installing") | ||
| cmd.Flags().StringVar(&flagVersion, "version", "", "override the version used for image tags (defaults to the binary version)") | ||
| cmd.Flags().StringVar(&imagePullPolicy, "image-pull-policy", "", "set imagePullPolicy on controller containers (e.g. Always, IfNotPresent, Never)") | ||
| cmd.Flags().BoolVar(&crd, "crd", false, "install CRDs along with the controller") |
There was a problem hiding this comment.
P0: The --crd flag default is changed from true to false, which is a breaking change that contradicts the PR description (which says default is true) and the README Quick Start guide (which says kelos install installs CRDs). Users running kelos install will silently skip CRD installation. If the intent is truly default=false, the README must be updated; if not, this default should be true.
(Based on your team's feedback about keeping documentation and config updated with changes.)
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At internal/cli/install.go, line 96:
<comment>The `--crd` flag default is changed from `true` to `false`, which is a breaking change that contradicts the PR description (which says default is `true`) and the README Quick Start guide (which says `kelos install` installs CRDs). Users running `kelos install` will silently skip CRD installation. If the intent is truly `default=false`, the README must be updated; if not, this default should be `true`.
(Based on your team's feedback about keeping documentation and config updated with changes.) </comment>
<file context>
@@ -95,7 +93,7 @@ func newInstallCommand(cfg *ClientConfig) *cobra.Command {
cmd.Flags().StringVar(&flagVersion, "version", "", "override the version used for image tags (defaults to the binary version)")
cmd.Flags().StringVar(&imagePullPolicy, "image-pull-policy", "", "set imagePullPolicy on controller containers (e.g. Always, IfNotPresent, Never)")
- cmd.Flags().BoolVar(&crd, "crd", true, "install CRDs (set to false to skip CRD installation)")
+ cmd.Flags().BoolVar(&crd, "crd", false, "install CRDs along with the controller")
return cmd
</file context>
| cmd.Flags().BoolVar(&crd, "crd", false, "install CRDs along with the controller") | |
| cmd.Flags().BoolVar(&crd, "crd", true, "install CRDs (set to false to skip CRD installation)") |
| }, | ||
| } | ||
|
|
||
| cmd.Flags().BoolVar(&crd, "crd", false, "remove CRDs along with the controller") |
There was a problem hiding this comment.
P2: The --crd flag default is changed from true to false for uninstall, contradicting the PR description which says default is true. While default=false is arguably the safer choice for uninstall (since deleting CRDs cascades to all custom resources), this contradicts the stated design. If default=false is intentional, the PR description needs correction; if not, restore true.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At internal/cli/install.go, line 183:
<comment>The `--crd` flag default is changed from `true` to `false` for uninstall, contradicting the PR description which says default is `true`. While `default=false` is arguably the safer choice for uninstall (since deleting CRDs cascades to all custom resources), this contradicts the stated design. If `default=false` is intentional, the PR description needs correction; if not, restore `true`.</comment>
<file context>
@@ -175,16 +173,14 @@ func newUninstallCommand(cfg *ClientConfig) *cobra.Command {
}
- cmd.Flags().BoolVar(&crd, "crd", true, "remove CRDs (set to false to keep CRDs in the cluster)")
+ cmd.Flags().BoolVar(&crd, "crd", false, "remove CRDs along with the controller")
return cmd
</file context>
Summary
--crdflag (defaultfalse) to theinstallcommand — CRDs are only installed when explicitly requested with--crd--crdflag (defaultfalse) to theuninstallcommand — CRDs are only removed when explicitly requested with--crdinstall --dry-run --crdincludes CRD manifests in outputMotivation
CRDs define custom resource types and deleting them cascades to all custom resources in the cluster. Making CRD install/uninstall opt-in (rather than the default) is a reasonable safety measure that gives users explicit control.
Test plan
TestInstallCommand_CRDFlagDefault— verifies default dry-run output excludes CRDsTestInstallCommand_CRDFlagTrue— verifies--crdincludes CRD manifests in dry-run outputTestUninstallCommand_CRDFlagAccepted— verifies the--crdflag is accepted by uninstallTestInstallCommand_DryRun— updated to match new default (no CRDs)make verifypassesmake testpassesCloses #461
🤖 Generated with Claude Code