-
Notifications
You must be signed in to change notification settings - Fork 314
Recreate ArgoCD interface for accessing Argo CD functions. #1170
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
Conversation
5a2a679 to
c3756b6
Compare
c3756b6 to
fbc44df
Compare
|
I've merged the other log-related PR into master, and rebased crd branch, hence the conflicts in this PR. Can you rebase your pr branch on the latest crd branch? |
a045d0f to
b4be96a
Compare
|
Thanks Cheng! I've updated go.mod with the new version of |
b4be96a to
b7b508e
Compare
pkg/kube/kubernetes.go
Outdated
| kc := &ImageUpdaterKubernetesClient{} | ||
| kc.KubeClient = kube.NewKubernetesClient(ctx, clientset, namespace) | ||
| kc.ApplicationsClientset = applicationsClientset | ||
| kc.ApplicationsClientSet = applicationsClientset |
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.
clientset is a convention used in many other places in argo-cd and kubernetes, I think we should keep it.
cmd/run.go
Outdated
| argocd.GetPrintableInterval(cfg.CheckInterval), | ||
| argocd.GetPrintableHealthPort(cfg.HealthPort), | ||
| setupLogger.Info("starting", | ||
| "app", version.BinaryName()+version.Version(), |
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.
there is no space between binary name and version. How about:
version.BinaryName(), version.Version(), which should produce something like "argocd-image-updater: v9.9.0"?
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 changed it to:
setupLogger.Info("starting",
"app", version.BinaryName()+": "+version.Version(),
that produces
time="2025-06-25T11:53:55+02:00" level=info msg=starting app="argocd-image-updater: v9.9.99+unknown" controller=imageupdater controllerGroup=argocd-image-updater.argoproj.io controllerKind=ImageUpdater healthport=8080 interval=2m0s logger=controller-setup loglevel=TRACE
|
|
||
| if tpl, err := template.New("commitMessage").Parse(commitMessageTpl); err != nil { | ||
| log.Fatalf("could not parse commit message template: %v", err) | ||
| setupLogger.Error(err, "could not parse commit message template") |
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.
should we return err here? Previously log.Fatalf would exit after logging, but setupLogger.Error() does not exit.
| cfg.KubeClient, err = argocd.GetKubeConfig(ctx, cfg.ArgocdNamespace, kubeConfig) | ||
| if err != nil { | ||
| log.Fatalf("could not create K8s client: %v", err) | ||
| setupLogger.Error(err, "could not create K8s client") |
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.
should we return err here? Previously log.Fatalf would exit after logging, but setupLogger.Error() does not exit.
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.
Yep, I forgot about it in few places.
pkg/argocd/argocd.go
Outdated
| } | ||
|
|
||
| // Type of the application | ||
| // ApplicationType Type of the applicationdisableKubernetes |
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.
"disableKubernetes" doesn't seem to belong here.
| // ListApplications returns a list of all application names that the API user | ||
| // has access to. | ||
| func (client *argoCD) ListApplications(labelSelector string) ([]v1alpha1.Application, error) { | ||
| func (client *argoCD) ListApplications(ctx context.Context, cr *api.ImageUpdater, labelSelector string) ([]v1alpha1.Application, error) { |
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.
should the param ctx be used below in line 334, instead of context.TODO()?
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.
Great catch! Updated.
Although I propose to remove this method for argoCD client. See GITOPS-7123.
|
also pls resolve lint errors in some tests. |
- Change signatures and implementations for GetApplication, ListApplications, UpdateSpec for type k8sClient. These functions now rely on controller-runtime client.Client. - Constructor NewK8SClient was simplified. - ArgoCD tests were updated. - `ListApplications` misses `labelSelector` feature (will be added in GITOPS-7119). - `GetApplication` misses `NamePattern` feature (will be added in GITOPS-7119). - bin/controller-gen* files were removed because they should be generated on each machine separately depending on arch, os, etc. - update go.mod Signed-off-by: Denis Karpelevich <[email protected]>
b7b508e to
754c4a7
Compare
…labs#1170) Signed-off-by: Denis Karpelevich <[email protected]>
…labs#1170) Signed-off-by: Denis Karpelevich <[email protected]>
ListApplicationsmisseslabelSelectorfeature (will be added in GITOPS-7119).GetApplicationmissesNamePatternfeature (will be added in GITOPS-7119).Log looks like this. Have a look at
logger=warmup-cacheorlogger=reconcilethat were propagated to the respectful functions.