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)
|
|
||
| ## Step 00 | ||
|
|
||
| Import a AddressScope that references other imported resources. The referenced imported resources have no matching resources yet. |
There was a problem hiding this comment.
Same NIT as before, I assume it's the scaffolding tool creating the README for the e2e tests, so up to you to if you want to go back and tweak the spelling. Again I'm splitting hairs here, please feel free to ignore.
There was a problem hiding this comment.
Done! Thanks for highlighting this.
winiciusallan
force-pushed
the
controller-addressscope
branch
from
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 👍 |
2 participants
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.Fixes #393