Skip to content

improved skill#711

Draft
eshulman2 wants to merge 1 commit intok-orc:mainfrom
eshulman2:improve_skills
Draft

improved skill#711
eshulman2 wants to merge 1 commit intok-orc:mainfrom
eshulman2:improve_skills

Conversation

@eshulman2
Copy link
Contributor

improve skills

@github-actions github-actions bot added the semver:patch No API change label Mar 15, 2026
go doc <gophercloud-module>.<Type>
go doc <gophercloud-module>.CreateOpts
```
- Note field types precisely: `int32` in gophercloud → `*int32` in status (not `*int`)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Always? Maybe I'm splitting hairs here, but a scenario where I don't believe we should have it as a pointer is when the zero-value doesn't matter. For instance, the new AddressScope controller has an IPVersion field returned by the API, and it should always be returned because we must specify it at creation and it can't be null. Here, the zero value does not matter since the possible values are only two: 4 or 6. Does it make sense?

@@ -198,24 +212,38 @@ Implement:
- `CreateResource()` - Build CreateOpts, call OpenStack API
- `DeleteResource()` - Call delete API
- `ListOSResourcesForImport()` - Apply filter to list results
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here, we also want to add all available fields for filtering. Wdyt?

**This step is required** - do not skip it.

Complete the test stubs in `internal/controllers/<kind>/tests/` and run tests following @.agents/skills/testing/SKILL.md
The scaffolding tool creates test directory stubs in `internal/controllers/<kind>/tests/` with README files explaining each test suite's purpose.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe it'd be good to include an orientation on implementing all kinds of tests that we provide. That is, e2e tests, unit tests via actuator_test.go for mutable fields and via apivalidation package for API validations.

Regarding the latter, @mandre pushed a PR making changes to the apivalidation package. If you all agree, it'd also be good to add them here as well.

@eshulman2
Copy link
Contributor Author

Hi @winiciusallan thanks for the very quick review, this should have been a draft but for some reason I am unable to convert it, I'll take a look at the comments but would mention this is still work in progress

@eshulman2 eshulman2 marked this pull request as draft March 16, 2026 09:20
@winiciusallan
Copy link
Contributor

Hi @winiciusallan thanks for the very quick review, this should have been a draft but for some reason I am unable to convert it, I'll take a look at the comments but would mention this is still work in progress

Oh, that's ok. Sorry if I reviewed it too soon.

@eshulman2
Copy link
Contributor Author

NP thanks for the review :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

semver:patch No API change

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants