-
Notifications
You must be signed in to change notification settings - Fork 5.4k
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
feat(cli): Add app diff option to specify exit code when diff #20144
Conversation
❌ Preview Environment deleted from BunnyshellAvailable commands (reply to this comment):
|
❌ Preview Environment deleted from BunnyshellAvailable commands (reply to this comment):
|
87c02bd
to
298fa12
Compare
@jannfis PTAL! |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #20144 +/- ##
=======================================
Coverage 55.97% 55.97%
=======================================
Files 322 322
Lines 44546 44548 +2
=======================================
+ Hits 24933 24937 +4
- Misses 17042 17047 +5
+ Partials 2571 2564 -7 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good stuff! Just a couple small requests.
cmd/argocd/commands/app.go
Outdated
@@ -1242,13 +1243,14 @@ func NewApplicationDiffCommand(clientOpts *argocdclient.ClientOptions) *cobra.Co | |||
proj := getProject(c, clientOpts, ctx, app.Spec.Project) | |||
foundDiffs := findandPrintDiff(ctx, app, proj.Project, resources, argoSettings, diffOption, ignoreNormalizerOpts) | |||
if foundDiffs && exitCode { | |||
os.Exit(1) | |||
os.Exit(diffExitCode) | |||
} | |||
}, | |||
} | |||
command.Flags().BoolVar(&refresh, "refresh", false, "Refresh application data when retrieving") | |||
command.Flags().BoolVar(&hardRefresh, "hard-refresh", false, "Refresh application data as well as target manifests cache") | |||
command.Flags().BoolVar(&exitCode, "exit-code", true, "Return non-zero exit code when there is a diff") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Want to update this flag's description to point out that it returns non-zero when there's an error?
cmd/argocd/commands/app.go
Outdated
} | ||
}, | ||
} | ||
command.Flags().BoolVar(&refresh, "refresh", false, "Refresh application data when retrieving") | ||
command.Flags().BoolVar(&hardRefresh, "hard-refresh", false, "Refresh application data as well as target manifests cache") | ||
command.Flags().BoolVar(&exitCode, "exit-code", true, "Return non-zero exit code when there is a diff") | ||
command.Flags().IntVar(&diffExitCode, "diff-exit-code", 1, "Return specified exit code when there is a diff") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I recall correctly, 20 is the typical exit code for errors. Would it make sense to mention that here so that the user can pick an exit code which is unlikely to conflict with exit codes for other conditions?
298fa12
to
7c10f5f
Compare
The argocd app diff command returns 1 if a difference is found. In related issues, they want to return an error code that is distinguishable from common errors. However, changing the existing behavior is likely to break user's automation code. So we want to provide an explicit option(--diff-exit-code) to specify an error code. Related: argoproj#3588 Signed-off-by: Eugene Kim <[email protected]>
7c10f5f
to
110f45e
Compare
@crenshaw-dev PTAL again!! |
The argocd app diff command returns 1 if a difference is found. In related issues, they want to return an error code that is distinguishable from common errors. However, changing the existing behavior is likely to break user's automation code. So we want to provide an explicit option(--diff-exit-code) to specify an error code.
Issue: #3588