Keystone: Implement Region controller - #862
Conversation
$ go run ./cmd/scaffold-controller -interactive=false \
-kind=Region \
-gophercloud-client=NewIdentityV3 \
-gophercloud-module=github.com/gophercloud/gophercloud/v2/openstack/identity/v3/regions \
-gophercloud-type=Region \
-openstack-json-object=region
On-behalf-of: SAP nils.gondermann@sap.com
Register with the resource generator On-behalf-of: SAP nils.gondermann@sap.com
Add the OpenStack client to scope On-behalf-of: SAP nils.gondermann@sap.com
Register the controller On-behalf-of: SAP nils.gondermann@sap.com
On-behalf-of: SAP nils.gondermann@sap.com
9fdcdd6 to
0c56e56
Compare
winiciusallan
left a comment
winiciusallan
left a comment
There was a problem hiding this comment.
Hey @gndrmnn, great work! At a first glance, this looks very good to me. I left a few comments, but I'm still missing a review in the tests. Will do it later.
0c56e56 to
d82f54a
Compare
On-behalf-of: SAP nils.gondermann@sap.com
On-behalf-of: SAP nils.gondermann@sap.com
383a3ed to
b8f077d
Compare
winiciusallan
left a comment
winiciusallan
left a comment
There was a problem hiding this comment.
Yup, looks very good. Thanks @gndrmnn! Just a nit comment, nothing blocking.
On-behalf-of: SAP nils.gondermann@sap.com
Generate the OLM bundle On-behalf-of: SAP nils.gondermann@sap.com
On-behalf-of: SAP nils.gondermann@sap.com
b8f077d to
d59d881
Compare
|
Is it possible to merge this one? We would like to introduce the management of regions on our site and are currently blocked by this one. |
|
@berendt I'm back, and I'll be looking at this very soon. |
mandre
left a comment
mandre
left a comment
There was a problem hiding this comment.
Looking good. We may just want to add a few comments clarifying the choices where they could lead to confusion, but otherwise ready to merge from my point of view.
| // If not specified, the name of the ORC object will be used. | ||
| // +optional | ||
| // +kubebuilder:validation:XValidation:rule="self == oldSelf",message="name is immutable" | ||
| Name *OpenStackName `json:"name,omitempty"` |
There was a problem hiding this comment.
Looking at keystone source code, the validation for region ID is:
region_id: dict[str, Any] = {
"type": ["string", "null"],
"minLength": 1,
"maxLength": 255,
"description": "The ID of the region.",
}
Which is in line with our OpenStackName type. I'd suggest adding a comment explaining why we're not useing KeystoneName here.
| }, | ||
| { | ||
| Name: "Region", | ||
| // Region has no "name" field, but the ID is used as the name |
There was a problem hiding this comment.
The combination of UsesNameAsID and IsNotNamed is highly confusing but it seems to do the trick.
Perhaps we should explain better in the comment how these two option can work together (because from the names, they shouldn't 😅).
| name: region-create-full | ||
| ref: region | ||
| assertAll: | ||
| - celExpr: "region.status.id != ''" |
There was a problem hiding this comment.
Nit: we can delete this line, as we're already checking below that the value is set to the name (thus non empty).
Actually, since the name is predictable, this doesn't need to be a CEL expression, we could even add this check to the resource matcher above. Not a big deal though, I understand we may want to keep this a CEL matcher for consistency with the other tests.
|
|
||
| // TODO: | ||
| // listOpts := regions.ListOpts{ | ||
| // ParentRegionID: ptr.Deref(resourceSpec.ParentRegionID), |
There was a problem hiding this comment.
Just an observation, the field in the spec would likely be called ParentRegionRef once we get around implementing it.
5 participants
Regions are weird in that they have an
idfield, which is used as a name (no UUID) but they lack a propernamefield.I took the liberty to do a
name->idmapping in the actuator and status as I think that makes more sense form a k-orc perspective. Though if you want I can rename the field toidjust to be consistent with the upstream naming.