-
Notifications
You must be signed in to change notification settings - Fork 5.2k
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: Enhance ArgoCD CLI: Dynamic Repo Server Retrieval with --core and --refresh Flags #17613
feat: Enhance ArgoCD CLI: Dynamic Repo Server Retrieval with --core and --refresh Flags #17613
Conversation
Signed-off-by: Mangaal <angommeeteimangaal@gmail.com>
…rver labels , if the label exist construct label selector PortForward Signed-off-by: Mangaal <angommeeteimangaal@gmail.com>
Signed-off-by: Mangaal <angommeeteimangaal@gmail.com>
repoServerPodLabelSelector := common.LabelKeyAppName + "=" + c.repoServerName | ||
repoServerName := c.repoServerName | ||
repoServererviceLabelSelector := common.LabelKeyComponentRepoServer + "=" + common.LabelValueComponentRepoServer | ||
repoServerServices, err := c.kubeClientset.CoreV1().Services(c.namespace).List(context.Background(), v1.ListOptions{LabelSelector: repoServererviceLabelSelector}) |
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.
Instead of deriving the argocd instance name from the services, can we not apply the label app.kubernetes.io/component=repo-server
to the spec.template.metadata
section of the repo server deployment and then use this label to get the right repo server.
https://github.com/argoproj/argo-cd/blob/f87897c53c6f04426953f5d3ca781d3240186a60/manifests/base/repo-server/argocd-repo-server-deployment.yaml#L16C9-L16C51
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.
Hi @anandf, I will update the implementation and add the labels "app.kubernetes.io/component=repo-server" to repo-server replica pods in the manifest
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.
Switching to app.kubernetes.io/component=repo-server
component based label might cause confusion when customer provides argument --repo-server-name
with the name of the argocd instance argocd-repo-server
. So let's keep the logic of deriving the argocd repo server name from the service name.
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.
Just a nit regarding the PR's title: Can you please try to find a shorter, more concise title that denotes what actually has been fixed or improved? :)
…cd-cli-core-flag
439f521
to
3b060b4
Compare
…o-cd into argocd-cli-core-flag
…mpty ClusterSharding{} Signed-off-by: Mangaal <angommeeteimangaal@gmail.com>
Signed-off-by: Mangaal <angommeeteimangaal@gmail.com>
Signed-off-by: Mangaal <angommeeteimangaal@gmail.com>
…o-cd into argocd-cli-core-flag
controller/cache/cache.go
Outdated
@@ -437,7 +437,7 @@ func (c *liveStateCache) getCluster(server string) (clustercache.ClusterCache, e | |||
return nil, fmt.Errorf("error getting cluster: %w", err) | |||
} | |||
|
|||
if !c.canHandleCluster(cluster) { | |||
if !c.canHandleCluster(cluster) && c.clusterSharding == nil { |
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.
I think the nil check should come first isn't it ?
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.
Hi @anandf, I have updated the code to handle nil ptr dereference separately
…o-cd into argocd-cli-core-flag
Signed-off-by: Mangaal <angommeeteimangaal@gmail.com>
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.
LGTM
cmd/argocd/commands/admin/app.go
Outdated
if repoServerAddress == "" { | ||
printLine("Repo server is not provided, trying to port-forward to argocd-repo-server pod.") | ||
overrides := clientcmd.ConfigOverrides{} | ||
repoServerPodLabelSelector := common.LabelKeyAppName + "=" + clientOpts.RepoServerName | ||
repoServerName := clientOpts.RepoServerName | ||
repoServererviceLabelSelector := common.LabelKeyComponentRepoServer + "=" + common.LabelValueComponentRepoServer |
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.
nit: repoServerServiceLabelSelector
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.
Thanks, @ishitasequeira, Updating the same.
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.
@Mangaal Just 1 nit. Rest looks good to me!!
…o-cd into argocd-cli-core-flag
Signed-off-by: Mangaal <angommeeteimangaal@gmail.com>
Signed-off-by: Mangaal <angommeeteimangaal@gmail.com>
…nd --refresh Flags (argoproj#17613) * add const key value for ComponentRepoServer Signed-off-by: Mangaal <angommeeteimangaal@gmail.com> * update NewRepoServerClient() to look for service with ComponentRepoServer labels , if the label exist construct label selector PortForward Signed-off-by: Mangaal <angommeeteimangaal@gmail.com> * add comment for the new constants Signed-off-by: Mangaal <angommeeteimangaal@gmail.com> * instead of passing nil which leads to nil ptr referance error, pass empty ClusterSharding{} Signed-off-by: Mangaal <angommeeteimangaal@gmail.com> * check for operator install repo server name Signed-off-by: Mangaal <angommeeteimangaal@gmail.com> * handle empty nil ptr dereference error Signed-off-by: Mangaal <angommeeteimangaal@gmail.com> * handle nil prt dereference Signed-off-by: Mangaal <angommeeteimangaal@gmail.com> * typo correction Signed-off-by: Mangaal <angommeeteimangaal@gmail.com> * run clidocsgen Signed-off-by: Mangaal <angommeeteimangaal@gmail.com> --------- Signed-off-by: Mangaal <angommeeteimangaal@gmail.com>
…nd --refresh Flags (argoproj#17613) * add const key value for ComponentRepoServer Signed-off-by: Mangaal <angommeeteimangaal@gmail.com> * update NewRepoServerClient() to look for service with ComponentRepoServer labels , if the label exist construct label selector PortForward Signed-off-by: Mangaal <angommeeteimangaal@gmail.com> * add comment for the new constants Signed-off-by: Mangaal <angommeeteimangaal@gmail.com> * instead of passing nil which leads to nil ptr referance error, pass empty ClusterSharding{} Signed-off-by: Mangaal <angommeeteimangaal@gmail.com> * check for operator install repo server name Signed-off-by: Mangaal <angommeeteimangaal@gmail.com> * handle empty nil ptr dereference error Signed-off-by: Mangaal <angommeeteimangaal@gmail.com> * handle nil prt dereference Signed-off-by: Mangaal <angommeeteimangaal@gmail.com> * typo correction Signed-off-by: Mangaal <angommeeteimangaal@gmail.com> * run clidocsgen Signed-off-by: Mangaal <angommeeteimangaal@gmail.com> --------- Signed-off-by: Mangaal <angommeeteimangaal@gmail.com>
Description
When using the ArgoCD CLI client with the --core and --refresh flags, it's essential to provide the
repo-server-name
argument for the default ArgoCD instance deployed with operators. However, in some cases, users might forget to specify this argument, leading to errors.Current Behavior:
Running the command
argocd app list --core
orargocd admin app get-reconcile-results --refresh text.txt
without providing the repo-server-name flag results in a default value ofargocd-repo-server
. This default value is not valid for ArgoCD installations with an operator. The actual repo server name is derived from the ArgoCD CR name, such as example-argocd-repo-server if the CR name is example-argocd. Consequently, an error occurs due to the mismatch between the default and actual repo server name.How to Reproduce the Issue:
To reproduce the issue:
You will encounter the following issue:
FATA[0000] cannot find pod with selector: [app.kubernetes.io/name=argocd-repo-server] - use the --{component}-name flag in this command or set the environmental variable (Refer to https://argo-cd.readthedocs.io/en/stable/user-guide/environment-variables), to change the Argo CD component name in the CLI
Fix:
This pull request enhances the ArgoCD CLI by dynamically fetching the correct repo server name instead of relying solely on a default value. It queries the ArgoCD component services with the label
app.kubernetes.io/component=repo-server
and updates the repo server name accordingly. If the service query fails, it falls back to the default value. This ensures compatibility with ArgoCD installations using operators and improves overall reliability.Additionally, this pull request addresses a bug documented in issue #17602
Output after fix:
Checklist: