-
Notifications
You must be signed in to change notification settings - Fork 183
Implement livestate for k8s multicluster plugin #5778
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
Implement livestate for k8s multicluster plugin #5778
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #5778 +/- ##
==========================================
+ Coverage 27.14% 27.19% +0.05%
==========================================
Files 508 508
Lines 53803 53942 +139
==========================================
+ Hits 14603 14671 +68
- Misses 38084 38155 +71
Partials 1116 1116 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Signed-off-by: Yoshiki Fujikane <[email protected]>
Signed-off-by: Yoshiki Fujikane <[email protected]>
dc50f71
to
0981d3e
Compare
Signed-off-by: Yoshiki Fujikane <[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.
Let's try
syncStates := make([]sdk.ApplicationSyncState, 0, len(targetConfigs)) | ||
for _, tc := range targetConfigs { | ||
// Get the kubectl tool path. | ||
kubectlPath, err := toolRegistry.Kubectl(ctx, cmp.Or(cfg.Spec.Input.KubectlVersion, tc.deployTarget.Config.KubectlVersion)) |
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.
[nits]
I think it's ok to fix this point in another PR.
This may be wrong. I think tc.deployTarget.Config.KubectlVersion
has a higher priority.
kubectlPath, err := toolRegistry.Kubectl(ctx, cmp.Or(cfg.Spec.Input.KubectlVersion, tc.deployTarget.Config.KubectlVersion)) | |
kubectlPath, err := toolRegistry.Kubectl(ctx, tc.deployTarget.Config.KubectlVersion, cmp.Or(cfg.Spec.Input.KubectlVersion)) |
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.
@Warashi
It seems that cfg.Spec.Input.KubectlVersion
has a higher priority in the old version.
// priority: applicationConfig.KubectlVersion > pipedConfig.KubectlVersion
https://github.com/pipe-cd/pipecd/blob/f53dd3cacc5dab5db3df48332b98f36b53158f17/pkg/app/piped/platformprovider/kubernetes/applier.go#L209C1-L217C2
It is the same as other deployment loginc for k8s plugin and k8s_multicluster.
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.
@ffjlabo
Oops, thank you. I am confused with deployTargets and multiTargets.
ref;
pipecd/pkg/app/pipedv1/plugin/kubernetes_multicluster/deployment/rollback.go
Lines 182 to 186 in f53dd3c
kubectlVersions := []string{cfg.Spec.Input.KubectlVersion, deployTargetConfig.KubectlVersion} | |
// If multi-target is specified, use the kubectl version specified in it. | |
if multiTarget != nil { | |
kubectlVersions = append([]string{multiTarget.KubectlVersion}, kubectlVersions...) | |
} |
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.
👌
What this PR does:
as title. This is a PoC.
Spec summary
TODOs
Why we need it:
We want to support livestate feature for multi cluster plugin.
Which issue(s) this PR fixes:
Part of #5006
Does this PR introduce a user-facing change?: