feat(cli): Support Server-Side Diff CLI #23978
Conversation
❌ Preview Environment deleted from BunnyshellAvailable commands (reply to this comment):
|
pjiang-dev
force-pushed
the
pjiang/cli-ssd
branch
from
fb8370e to
2530035
Compare
nitishfy
left a comment
nitishfy
left a comment
There was a problem hiding this comment.
can you please fix the failing checks?
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #23978 +/- ##
==========================================
- Coverage 60.25% 60.10% -0.15%
==========================================
Files 347 347
Lines 59354 59853 +499
==========================================
+ Hits 35761 35976 +215
- Misses 20724 20987 +263
- Partials 2869 2890 +21 ☔ View full report in Codecov by Sentry. |
pjiang-dev
force-pushed
the
pjiang/cli-ssd
branch
from
7e3a8f2 to
4a67bbe
Compare
cmd/argocd/commands/app.go
Outdated
| foundDiffs := findAndPrintServerSideDiff(ctx, app, proj.Project, resources, argoSettings, diffOption, appIf, appName, appNs) | ||
| if foundDiffs && exitCode { | ||
| os.Exit(diffExitCode) | ||
| } | ||
| return |
There was a problem hiding this comment.
It doesn't seem like a good idea to create a new function just for server-side diff. Note that findandPrintDiff is also called in the sync command which wouldn't have the SSD feature. I think that we should modify the findandPrintDiff function and add the SSD feature there based on the diff options provided.
There was a problem hiding this comment.
Good catch. I did not realize this findandPrintDiff is used in sync command as well
There was a problem hiding this comment.
Question @leoluz. For the sync command, are we going to infer whether to use server-side diff in the result by using the app's annotation only?
Or will we need a new flag for sync as well for --server-side-diff ?
For example something like argocd app sync myapp --dry-run --server-side ?
I have currently hardcoded it to false until we decide
There was a problem hiding this comment.
are we going to infer whether to use server-side diff in the result by using the app's annotation only?
Or will we need a new flag for sync as well for --server-side-diff ?
Can we have both? I think by default it should be checking what is in the app's annotation. However it would be nice if from the CLI we could customize this behaviour as well. I would say: Make sure to honour what is configured in the app is the priority #1. Adding a new flag is a nice to have but lower priority.
Signed-off-by: Peter Jiang <[email protected]>
Signed-off-by: Peter Jiang <[email protected]>
Signed-off-by: Peter Jiang <[email protected]>
Signed-off-by: Peter Jiang <[email protected]>
Signed-off-by: Peter Jiang <[email protected]>
Signed-off-by: Peter Jiang <[email protected]>
Signed-off-by: Peter Jiang <[email protected]>
Signed-off-by: Peter Jiang <[email protected]>
Signed-off-by: Peter Jiang <[email protected]>
Signed-off-by: Peter Jiang <[email protected]>
Signed-off-by: Peter Jiang <[email protected]>
Signed-off-by: Peter Jiang <[email protected]>
Signed-off-by: Peter Jiang <[email protected]>
Signed-off-by: Peter Jiang <[email protected]>
Signed-off-by: Peter Jiang <[email protected]>
Signed-off-by: Peter Jiang <[email protected]>
Signed-off-by: Peter Jiang <[email protected]>
Signed-off-by: Peter Jiang <[email protected]>
pjiang-dev
force-pushed
the
pjiang/cli-ssd
branch
from
681fc57 to
5255c30
Compare
agaudreault
left a comment
agaudreault
left a comment
There was a problem hiding this comment.
overall LGTM, few nitpicks
Signed-off-by: Peter Jiang <[email protected]>
| // Get cluster connection for server-side dry run | ||
| cluster, err := argo.GetDestinationCluster(ctx, a.Spec.Destination, s.db) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("error getting destination cluster: %w", err) | ||
| } | ||
|
|
||
| clusterConfig, err := cluster.RawRestConfig() | ||
| if err != nil { | ||
| return nil, fmt.Errorf("error getting cluster raw REST config: %w", err) | ||
| } | ||
|
|
||
| // Create server-side diff dry run applier | ||
| openAPISchema, gvkParser, err := s.kubectl.LoadOpenAPISchema(clusterConfig) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to get OpenAPI schema: %w", err) | ||
| } | ||
|
|
||
| applier, cleanup, err := kubeutil.ManageServerSideDiffDryRuns(clusterConfig, openAPISchema, func(_ string) (kube.CleanupFunc, error) { | ||
| return func() {}, nil | ||
| }) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("error creating server-side dry run applier: %w", err) | ||
| } |
There was a problem hiding this comment.
This logic is similar to appStateManager.getServerSideDiffDryRunApplier (in controller/sync.go). Can you please verify if we can extract the similar logic from both and centralize it in a single function?
There was a problem hiding this comment.
They do look similar but are different workflows with different patterns.
Controller uses liveStateCache.GetClusterCache(cluster) to get a cached cluster object
and openAPISchema is already loaded and cached
Server uses argo.GetDestinationCluster() to get a fresh cluster object and openAPI schema must be loaded fresh via s.kubectl.LoadOpenAPISchema(clusterConfig)
Controller: Needs RawRestConfig() only for the applier creation
Server: Needs RawRestConfig() twice - once for schema loading AND once for applier creation
So centralizing the logic would require abstracting cache vs. direct cluster acces and i don't think this is worth the complexity
leoluz
left a comment
leoluz
left a comment
There was a problem hiding this comment.
Replied #23978 (comment)
PTAL
leoluz
left a comment
leoluz
left a comment
There was a problem hiding this comment.
We need to make sure that the documentation of this new command is created in ./docs/user-guide/commands. I believe that there is a make target available for updating that folder.
Signed-off-by: Peter Jiang <[email protected]>
Signed-off-by: Peter Jiang <[email protected]>
Signed-off-by: Peter Jiang <[email protected]>
Signed-off-by: Peter Jiang <[email protected]>
Signed-off-by: Peter Jiang <[email protected]> feat(azure): allow setting azure cloud (gov, china) for authentication Signed-off-by: Yohan Belval <[email protected]> feat(azure): allow setting azure cloud (gov, china) for authentication Signed-off-by: Yohan Belval <[email protected]>
Signed-off-by: Peter Jiang <[email protected]> feat(azure): allow setting azure cloud (gov, china) for authentication Signed-off-by: Yohan Belval <[email protected]> feat(azure): allow setting azure cloud (gov, china) for authentication Signed-off-by: Yohan Belval <[email protected]>
Signed-off-by: Peter Jiang <[email protected]>
Signed-off-by: Peter Jiang <[email protected]> Signed-off-by: enneitex <[email protected]>
Signed-off-by: Peter Jiang <[email protected]>
Signed-off-by: Peter Jiang <[email protected]> Signed-off-by: Mangaal <[email protected]>
Signed-off-by: Peter Jiang <[email protected]>
Signed-off-by: Peter Jiang <[email protected]>
Signed-off-by: Peter Jiang <[email protected]>
4 participants
closes #23868
fixes #22345
fixes #23171
Adds CLI support for argocd app diff
--server-side-diffand works with existing flags--local --revisionChecklist: