-
Notifications
You must be signed in to change notification settings - Fork 87
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 "upgrade plan" support #408
✨ Implement "upgrade plan" support #408
Conversation
/hold |
c2bf71a
to
731f8c8
Compare
731f8c8
to
828e437
Compare
/retest |
2 similar comments
/retest |
/retest |
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.
Looks good to me, just some questions/suggestions/nits
@@ -375,100 +372,6 @@ func generateCAPIOperatorDeployment(name, namespace string) *appsv1.Deployment { | |||
} | |||
} |
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.
One question - this test removed, is it covered by upgrade plan tests? Shouldn't we leave both for clarity?
g.Eventually(func() (bool, error) { | ||
provider, err := getGenericProvider(ctx, env, string(util.ClusterctlProviderType(genericProvider)), genericProvider.GetName(), genericProvider.GetNamespace()) | ||
if err != nil { | ||
return false, err | ||
} | ||
|
||
if provider.GetSpec().FetchConfig == nil || provider.GetSpec().FetchConfig.URL != tt.customURL { | ||
return false, nil | ||
} | ||
|
||
return true, nil | ||
}, waitShort).Should(BeTrue()) |
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.
This format makes things a bit more tidy IMO.
g.Eventually(func() (bool, error) { | |
provider, err := getGenericProvider(ctx, env, string(util.ClusterctlProviderType(genericProvider)), genericProvider.GetName(), genericProvider.GetNamespace()) | |
if err != nil { | |
return false, err | |
} | |
if provider.GetSpec().FetchConfig == nil || provider.GetSpec().FetchConfig.URL != tt.customURL { | |
return false, nil | |
} | |
return true, nil | |
}, waitShort).Should(BeTrue()) | |
g.Eventually(func(g g.Gomega) error { | |
provider, err := getGenericProvider(ctx, env, string(util.ClusterctlProviderType(genericProvider)), genericProvider.GetName(), genericProvider.GetNamespace()) | |
g.Expect(err).ToNot(HaveOccurred()) | |
g.Expect(provider.GetSpec().FetchConfig != nil && provider.GetSpec().FetchConfig.URL == tt.customURL).To(BeTrue()) | |
}, waitShort).Should(Succeed()) |
/area plugin |
8c8298b
to
0c92b56
Compare
✅ Deploy Preview for kubernetes-sigs-cluster-api-operator ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
0c92b56
to
ac389d7
Compare
/hold cancel |
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.
/approve
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: alexander-demicev The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
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
/hold
To give time to resolve comments from @Danil-Grigorev
LGTM label has been added. Git tree hash: e3e00b12399b678b886855d9eb15b1a05140853c
|
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.
One question to runtime extension provider addition, otherwise lgtm.
This test overwrites exiting CRDs with older versions, which breaks futher tests.
ac389d7
to
7e6e7eb
Compare
/lgtm Will leave hold in case Mike wants to re-add removed test, otherwise everything looks good. Thanks! |
LGTM label has been added. Git tree hash: d5797ef9378eaa039619667879372b7ee70df2a3
|
/hold cancel |
/retest |
What this PR does / why we need it:
This PR implements
upgrade plan
support for the pluginWhich issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)
format, will close the issue(s) when PR gets merged):Fixes #129