✨ Move subnet options to SubnetSpec #1856
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
k8s-ci-robot
added
do-not-merge/hold
✅ Deploy Preview for kubernetes-sigs-cluster-api-openstack ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
k8s-ci-robot
added
needs-rebase
k8s-ci-robot
removed
the
needs-rebase
MaysaMacedo
reviewed
Feb 2, 2024
EmilienM
reviewed
Feb 2, 2024
dulek
force-pushed
the
subnet-spec
branch
3 times, most recently
from
f029840
to
f063a8b
Compare
3 tasks
|
I made a small update of unnecessary restore and added CRD changes docs. |
|
/test pull-cluster-api-provider-openstack-e2e-full-test |
|
/lgtm |
k8s-ci-robot
added
the
lgtm
EmilienM
reviewed
Feb 5, 2024
| // If you leave this empty, no network will be created. | ||
| NodeCIDR string `json:"nodeCidr,omitempty"` | ||
| // +kubebuilder:validation:MaxItems=1 | ||
| NodeSubnets []SubnetSpec `json:"nodeSubnets,omitempty"` | ||
|
|
||
| // If NodeCIDR is set this option can be used to detect an existing router. |
There was a problem hiding this comment.
We need to update these comments where NodeCIDR is mentioned.
Also re-reviewing this, I realized that we also have Network, Subnets, Router.
Maybe to avoid confusion we could rename NodeSubnets to ManagedSubnets in the same vain as ManagedSecurityGroups (see PR).
There was a problem hiding this comment.
Yeah, that's not a bad idea. I've applied it, let's see if correctly.
|
/retitle ✨ Move subnet options to SubnetSpec |
k8s-ci-robot
changed the title
k8s-ci-robot
removed
the
lgtm
|
Looks like it works! |
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
/lgtm |
mdbooth
approved these changes
Feb 14, 2024
| // network, a subnet with NodeCIDR, and a router connected to this subnet. | ||
| // If you leave this empty, no network will be created. | ||
| NodeCIDR string `json:"nodeCidr,omitempty"` | ||
| // ManagedSubnets are describing OpenStack Subnets to be created. Cluster actuator |
There was a problem hiding this comment.
nit
Suggested change
| // ManagedSubnets are describing OpenStack Subnets to be created. Cluster actuator | |
| // ManagedSubnets describe OpenStack Subnets to be created. The cluster controller |
| @@ -539,6 +539,8 @@ func reconcileNetworkComponents(scope scope.Scope, cluster *clusterv1.Cluster, o | |||
| handleUpdateOSCError(openStackCluster, fmt.Errorf("failed to reconcile router: %w", err)) | |||
| return fmt.Errorf("failed to reconcile router: %w", err) | |||
| } | |||
| } else { | |||
| return fmt.Errorf("failed to reconcile network: ManagedSubnets only supports one element, %d provided", len(openStackCluster.Spec.ManagedSubnets)) | |||
There was a problem hiding this comment.
Reviewer note: confirmed this is also checked at admission as maxitems is specified in the CRD.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dulek, mdbooth 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 |
k8s-ci-robot
added
the
approved
k8s-ci-robot
removed
the
lgtm
|
/hold cancel |
k8s-ci-robot
removed
the
do-not-merge/hold
|
/lgtm |
k8s-ci-robot
added
the
lgtm
This commit introduces SubnetSpec field onto the OpenStackClusterSpec that is supposed to hold all options related to subnets created by CAPO. This means nodeCidr and DNSNameservers are moved into that struct.
k8s-ci-robot
removed
the
lgtm
|
/lgtm |
k8s-ci-robot
added
the
lgtm
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What this PR does / why we need it:
This commit introduces SubnetSpec field onto the OpenStackClusterSpec that is supposed to hold all options related to subnets created by CAPO. This means nodeCidr and DNSNameservers are moved into that struct.
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 #
Special notes for your reviewer:
TODOs:
/hold