Manilla: Share Type Controller#681
Conversation
…ntroller go run ./cmd/scaffold-controller -interactive=false \ -kind=ShareType \ -gophercloud-client=NewSharedFilesystemV2 \ -gophercloud-module=github.com/gophercloud/gophercloud/v2/openstack/sharedfilesystems/v2/sharetypes \ -gophercloud-type=ShareType \ -openstack-json-object=share_type Signed-off-by: Daniel Lawton <dlawton@redhat.com>
dlaw4608
force-pushed
the
manilla_share_types
branch
from
8316dcd to
8bfbe95
Compare
dlaw4608
force-pushed
the
manilla_share_types
branch
from
8bfbe95 to
fc60408
Compare
- E2E tests included - API configured Signed-off-by: Daniel Lawton <dlawton@redhat.com>
dlaw4608
force-pushed
the
manilla_share_types
branch
from
fc60408 to
4d72a18
Compare
winiciusallan
left a comment
winiciusallan
left a comment
There was a problem hiding this comment.
Hi @dlaw4608, hanks for adding this controller! I've done a round of review through your changes and added some comments inline. Haven't took a look at the tests yet.
Although things are working, I have a feeling that would be better to address some things on gophercloud's side, but I would like to hear from you and @mandre. How does that sound to you?
| package v1alpha1 | ||
|
|
||
| // ShareTypeResourceSpec contains the desired state of the resource. | ||
| // All fields are immutable after creation. |
There was a problem hiding this comment.
This is because Gophercloud currently does not implement the update operation for Share types, right? If this is the case, +1 for keeping it as immutable.
| // driverHandlesShareServers defines the driver mode for share server, or storage, life cycle management. | ||
| // This is a required extra specification for share types. | ||
| // +kubebuilder:default:=true | ||
| // +optional |
There was a problem hiding this comment.
Per the API documentation, this field should be required.
When creating a new share type, required extra-specifications must be provided. driver_handles_share_servers is a required extra-specification [...]
Also, considering this field as required, I think we may want to remove the default value and let the user pass it.
| apiservicedefinitions: {} | ||
| customresourcedefinitions: | ||
| owned: | ||
| - description: AddressScope is the Schema for an ORC resource. |
There was a problem hiding this comment.
I forgot to generate the bundle 😅 for the AddressScope controller. This was added in ee7e2a3. A rebase may sync things.
|
|
||
| # Optional: determines if the storage backend manages share servers (default: true) | ||
| # true = driver handles share server lifecycle | ||
| # false = driver does not handle share servers (admin must configure) |
There was a problem hiding this comment.
If you are changing driverHandlesShareServers to be required, remember to update those :)
| // TODO(scaffolding): Add more resource-specific validation tests. | ||
| // Some common things to test: | ||
| // - Immutability of fields with `self == oldSelf` validation | ||
| // - Enum validation (valid and invalid values) | ||
| // - Numeric range validation (min/max bounds) | ||
| // - Tag uniqueness (if the resource has tags with listType=set) | ||
| // - Format validation (CIDR, UUID, etc.) | ||
| // - Cross-field validation rules |
There was a problem hiding this comment.
Scaffolding controller leftovers. Let's remove them.
Since we're here, what do you think of adding a validation to check the spec immutability?
| } | ||
|
|
||
| func (actuator sharetypeActuator) GetOSResourceByID(ctx context.Context, id string) (*osResourceT, progress.ReconcileStatus) { | ||
| // ShareTypes don't have a Get by ID API, so we list and filter |
| // Build extra specs manually because gophercloud's ExtraSpecsOpts doesn't handle | ||
| // boolean false values correctly with the required:"true" tag |
There was a problem hiding this comment.
Yeah... We need to change the DriverHandlesShareServers type to be a pointer so we can accept false as a valid value.
This would be a breaking change, so I'm okay using this approach, but it would be good to add a TODO comment so we don't forget to change it in the future.
2 participants
Closes this issue: #680