Conversation
go run ./cmd/scaffold-controller \
-interactive=false \
-kind AddressScope \
-gophercloud-client NewNetworkV2 \
-gophercloud-module github.com/gophercloud/gophercloud/v2/openstack/networking/v2/extensions/layer3/addressscopes \
-optional-create-dependency Project \
-import-dependency Project
|
Hey @winiciusallan great job!! looks to me like its almost ready to be merged, added a couple of nits, also don't forget to add the new controller to the list of implemented controllers in the README.md!! |
|
|
||
| ## Step 00 | ||
|
|
||
| Create a AddressScope using all available fields, and verify that the observed state corresponds to the spec. |
There was a problem hiding this comment.
Tiny NIT, technically should be "an AddressScope" instead of "a AddressScope", "an" is used for words starting with a vowel sound ("an endpoint"), "a" is for words starting with a consonant sound ("a domain"), again this makes no difference to the code just thought I would point it out.
There was a problem hiding this comment.
Thanks for noting this, Daniel! I've searched for more occurrences of this spelling error and found more. I'll fix all of them and push the changes.
❯ grep -rni "a AddressScope" internal/controllers/addressscope/ | cut -d ':' -f1
internal/controllers/addressscope/tests/addressscope-update/README.md
internal/controllers/addressscope/tests/addressscope-create-minimal/README.md
internal/controllers/addressscope/tests/addressscope-create-full/README.md
internal/controllers/addressscope/tests/addressscope-create-full/README.md
internal/controllers/addressscope/tests/addressscope-import/README.md
internal/controllers/addressscope/tests/addressscope-import/README.md
internal/controllers/addressscope/tests/addressscope-import/README.md
internal/controllers/addressscope/tests/addressscope-import-dependency/README.md
internal/controllers/addressscope/tests/addressscope-import-dependency/README.md
internal/controllers/addressscope/tests/addressscope-import-dependency/README.md
There was a problem hiding this comment.
Actually, I noticed that it is also occurring at the endpoint controller. I'll open a PR soon to fix this as well.
(I don't know if it's worth it, but we might include in AGENTS.md guidance about spelling errors, and ask for them to be fixed as soon as they're found)
There was a problem hiding this comment.
We could actually add a fix to the scaffolding tool to generate grammatically correct README files.
internal/controllers/addressscope/tests/addressscope-import-dependency/README.md
Outdated
Show resolved
Hide resolved
ace4ca4 to
65434eb
Compare
|
@dlaw4608 I've addressed your nit comments. Could you check? I've also added to the README the new controller as implemented. |
|
LGTM thanks 👍 |
mandre
left a comment
There was a problem hiding this comment.
I'm curious about what you say regarding setting shared from true to false. We should look into it and fix it, either here, in gophercloud, or in openstack.
| // this value. | ||
| // +optional | ||
| // +kubebuilder:validation:XValidation:rule="self == oldSelf",message="shared is immutable" | ||
| Shared *bool `json:"shared,omitempty"` |
There was a problem hiding this comment.
Although the Shared field is mutable in the API, I've decided to keep it as immutable because we can't update true -> false
Is that right? It's both a pointer here and in gophercloud, so we should be able to pass it a value of false when updating the field. Do you recall what the problem was exactly?
There was a problem hiding this comment.
Yes. The error is with those lines. Looks like something to change in OpenStack (if it really makes sense to change).
Taking a quick look at the feature spec[1] and at the first commit review[2] for this feature, I didn't find any rationale to have this as it is.
[1] https://specs.openstack.org/openstack/neutron-specs/specs/mitaka/address-scopes.html
[2] https://review.opendev.org/c/openstack/neutron/+/189741?tab=comments
There was a problem hiding this comment.
If you are okay with this, I can bring this discussion to the next neutron meeting and double-check with the maintainers.
There was a problem hiding this comment.
Oh ok, thanks for the pointer. It looks intentional. In that case, we should add a comment explaining our choice.
Or alternatively, we make it mutable, but add a CEL validation that prevents unsharing a shared address scope.
There was a problem hiding this comment.
Ok. I made the field mutable, but added a validation to prevent unsharing the address scope. I think it is better to comply with the OpenStack behavior and give the operator the ability to share it if they want to.
About the addressscope-update, I added a new step file to create an AddressScope so we can change the share field later, but not revert it on 02-reverted-resource.yaml.
Actually, I think it'll be cleaner if we could capture the CEL validation and ensure that it's throwing properly. About this, I thought of capturing the output of the command in 02-reverted-resource.yaml and checking for the same message as in the CEL expression. Wdyt?
There was a problem hiding this comment.
Or we just test CEL validation via test/apivalidations. Whatever is simpler.
I like using test/apivalidations because that provides more flexibility, and we already have that in place.
| ref: addressscope | ||
| assertAll: | ||
| - celExpr: "!has(addressscope.status.resource.description)" | ||
| # --- |
There was a problem hiding this comment.
Let's remove this if it's unused.
There was a problem hiding this comment.
Done. I've uncommented and added a check for non-empty projectID
| ref: addressscope | ||
| assertAll: | ||
| - celExpr: "!has(addressscope.status.resource.description)" | ||
| # --- |
There was a problem hiding this comment.
Ditto, we shouldn't leave commented code if we're never going to uncomment in.
There was a problem hiding this comment.
Done. I've uncommented and added a check for non-empty projectID as well.
65434eb to
2bfd970
Compare
| // this value. We can't unshared a shared address scope; Neutron | ||
| // enforces this. | ||
| // +optional | ||
| // +kubebuilder:validation:XValidation:rule="!(oldSelf && !self)",message="shared address scope can't be unshared" |
There was a problem hiding this comment.
We haven't been really good at consistently writing API validation tests for our controller. Ideally we should have one for this field because the CEL validation is getting complex.
(I've just asked claude to write the missing tests, we'll see what it's going to come up with 😅)
| // this value. | ||
| // +optional | ||
| // +kubebuilder:validation:XValidation:rule="self == oldSelf",message="shared is immutable" | ||
| Shared *bool `json:"shared,omitempty"` |
There was a problem hiding this comment.
Or we just test CEL validation via test/apivalidations. Whatever is simpler.
I like using test/apivalidations because that provides more flexibility, and we already have that in place.
|
|
||
| } | ||
| } | ||
| // func TestHandleDescriptionUpdate(t *testing.T) { |
There was a problem hiding this comment.
Oups, we forgot some more commented code. Could you remove the TestHandleDescriptionUpdate() test, and add one for the Shared field?
This PR introduces the AddressScope controller.
Notes for the reviewers:
true -> false; this way, we can't unset the field once set as true. IMO it is safer to keep it as immutable. If anyone has a different opinion, we can discuss.For #393