-
Notifications
You must be signed in to change notification settings - Fork 198
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
Allow configuration of API server instance group name #1207
Allow configuration of API server instance group name #1207
Conversation
Hi @bfournie. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
✅ Deploy Preview for kubernetes-sigs-cluster-api-gcp ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
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.
/ok-to-test
f05d645
to
141c227
Compare
name: "instanceGroup name is overridden (should create instanceGroup)", | ||
scope: func() Scope { | ||
tagOverride := "master" | ||
clusterScope.GCPCluster.Spec.LoadBalancer = infrav1.LoadBalancerSpec{ |
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.
Isn't this changing a variable in a shared scope? If the tests are run in a random order, couldn't this set clusterScope.GCPCluster.Spec.LoadBalancer
for the rest of the suite?
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.
Good catch. Changed to use a separate scope to avoid overwriting.
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! That'll work.
Another pattern I've seen in tests for other providers is for the function to take in the "golden" one as an argument and return a new copy with the modifications.
Something like
scope: func(s Scope) Scope {
s.GCPCluster.Spec.LoadBalancer = infrav1.LoadBalancerSpec{....}
return s
}
and
scope: func(s Scope) Scope {
return s
}
That way, you get to have the top-level one remain clean and passed in on each invocation of test.scope
to get a fresh, mutable copy.
Up to you if you want to use that here or not.
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.
Good suggestion. I've reworked it to make the mods only in the test which saves some duplication.
141c227
to
af5af05
Compare
af5af05
to
be92b73
Compare
cloud/scope/machine.go
Outdated
tag := infrav1.APIServerRoleTagValue | ||
if m.ClusterGetter.LoadBalancer().APIServerInstanceGroupTagOverride != nil { | ||
tag = *m.ClusterGetter.LoadBalancer().APIServerInstanceGroupTagOverride | ||
} |
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.
same nit/suggestion to consider using ptr.Deref
2c5eab7
to
116f171
Compare
/retest |
116f171
to
f740f56
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.
This looks good to me, thanks @bfournie . There is one nit.
As the API diff is related to the exported go packages and not the API definition:
/override pull-cluster-api-provider-gcp-apidiff
/approve
@richardcase: Overrode contexts on behalf of richardcase: pull-cluster-api-provider-gcp-apidiff In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bfournie, richardcase 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 |
f740f56
to
42a9f03
Compare
Provides the ability to override the API server instance group name to allow compatibility when using the Openshift Machine API.
42a9f03
to
318600d
Compare
@bfournie: The following test failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed 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.
/lgtm
Thanks @bfournie !
/tide refresh |
What type of PR is this?
/kind feature
What this PR does / why we need it:
Provides the ability to override the API server instance group name to allow compatibility when using the Openshift Machine API.
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)
format, will close the issue(s) when PR gets merged):Fixes #1194
Special notes for your reviewer:
Note that a new
LoadBalancerSpec
field has been added toGCPClusterSpec
. Although this only contains one field it will be populated in the future with additional fields in order to support a LoadBalancer for private clusters.TODOs:
Release note: