-
Notifications
You must be signed in to change notification settings - Fork 278
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: support managing Application CRD using K8S client (#140) #144
Conversation
Codecov Report
@@ Coverage Diff @@
## master #144 +/- ##
==========================================
- Coverage 68.12% 68.07% -0.05%
==========================================
Files 19 19
Lines 1302 1322 +20
==========================================
+ Hits 887 900 +13
- Misses 336 340 +4
- Partials 79 82 +3
Continue to review full report at Codecov.
|
912c196
to
23296f1
Compare
…s#140) Signed-off-by: Alexander Matyushentsev <[email protected]>
23296f1
to
70e1f91
Compare
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 @alexmt! LGTM, with just one nitpicker comment.
cmd/main.go
Outdated
var argoClient argocd.ArgoCD | ||
switch cfg.ApplicationsAPIKind { | ||
case applicationsAPIKindK8S: | ||
argoClient = argocd.NewK8SClient(cfg.KubeClient) |
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.
Will this never return an error?
pkg/argocd/argocd.go
Outdated
|
||
// NewAPIClient creates a new API client for ArgoCD and connects to the ArgoCD | ||
// API server. | ||
func NewK8SClient(kubeClient *kube.KubernetesClient) ArgoCD { |
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.
To be consistent with NewAPIClient
, I think we should have the return value specified as (ArgoCD, error)
, even if we currently just return nil
here.
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.
Agree, we might need to return error in a future. Applied your suggestion.
Signed-off-by: Alexander Matyushentsev <[email protected]>
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! LGTM!
…s#140) (argoproj-labs#144) * feat: support managing Application CRD using K8S client (argoproj-labs#140) Signed-off-by: Alexander Matyushentsev <[email protected]> * apply review notes Signed-off-by: Alexander Matyushentsev <[email protected]>
Signed-off-by: Alexander Matyushentsev [email protected]
Closes #140
PR implements proposal from #140:
--argocd-namespace
flag--applications-api argocd
flag (orAPPLICATIONS_API
env variable).