Keystone: Group Controller #597
Conversation
winiciusallan
left a comment
winiciusallan
left a comment
There was a problem hiding this comment.
To me, it is very close to a merge state. Really well done, Daniel. Thanks!
internal/controllers/group/status.go
Outdated
| // TODO(scaffolding): add all of the fields supported in the GroupResourceStatus struct | ||
| // If a zero-value isn't expected in the response, place it behind a conditional |
There was a problem hiding this comment.
Maybe you have forgot? If you're still working, ignore it.
There was a problem hiding this comment.
I missed that one, Thanks @winiciusallan 👍
| ref: group | ||
| assertAll: | ||
| - celExpr: "group.status.id != ''" | ||
| - celExpr: "!has(group.status.resource.description)" |
There was a problem hiding this comment.
Just to confirm, KUTTL does not catch if a property is not present just matching with the declared status, right? Is that why you added this expression?
There was a problem hiding this comment.
- celExpr: "!has(group.status.resource.description)" will explicitly check that the description field does not exist in the actual resource, since leaving it out of the expected state alone is not enough. So yes because KUTTL doesn't catch if a field is missing, is exactly the reason this CEL assertion is needed.
There was a problem hiding this comment.
Ack. Thanks for the explanation.
There was a problem hiding this comment.
Indeed, this is a common pattern we're already using in our tests.
| kind: Group | ||
| metadata: | ||
| name: group-dependency-no-domain | ||
| status: |
There was a problem hiding this comment.
What do you think of checking this after dependency creation?
assertAll:
- celExpr: "group.status.resource.domainID == domain.status.id"There was a problem hiding this comment.
Sounds good 👍 will add it in to verify that the Group resource picks up the real domain.status.id
dlaw4608
force-pushed
the
keystone_group
branch
from
b26e0d1 to
740e9cf
Compare
|
Failed to assess the semver bump. See logs for details. |
dlaw4608
force-pushed
the
keystone_group
branch
from
740e9cf to
1fa78a7
Compare
|
Failed to assess the semver bump. See logs for details. |
CLI Command: go run ./cmd/scaffold-controller -interactive=false \ -kind Group \ -gophercloud-client NewIdentityV3 \ -gophercloud-module github.com/gophercloud/gophercloud/v2/openstack/identity/v3/groups \ -import-dependency Domain \ -optional-create-dependency Domain Signed-off-by: Daniel Lawton <dlawton@redhat.com>
dlaw4608
force-pushed
the
keystone_group
branch
from
1fa78a7 to
b11149c
Compare
| @@ -1,28 +1,23 @@ | |||
|
|
|||
There was a problem hiding this comment.
Just curious, was this empty line generated by the scaffolding tool? If so we'll want to fix the template because this will generate an obscure warning when running the kuttl tests. See #580.
There was a problem hiding this comment.
Tested it out and yep! the scaffold is the culprit, will look more into why at a later time 👍
| ref: group | ||
| assertAll: | ||
| - celExpr: "group.status.id != ''" | ||
| - celExpr: "!has(group.status.resource.description)" |
There was a problem hiding this comment.
Indeed, this is a common pattern we're already using in our tests.
| kind: Domain | ||
| name: group-dependency | ||
| ref: domain | ||
| - apiVersion: v1 |
There was a problem hiding this comment.
Just a nit: we do not need to get the secret anymore in this step.
| @@ -0,0 +1,8 @@ | |||
| --- | |||
| apiVersion: openstack.k-orc.cloud/v1alpha1 | |||
There was a problem hiding this comment.
Have you considered disabling this domain at the same time as the other one? This would make the operation run in parallel rather than sequentially.
There was a problem hiding this comment.
Good idea, changes made 👍
There was a problem hiding this comment.
Why was the import-dependency test removed? Was it not relevant? If so you should explain in the commit message why.
There was a problem hiding this comment.
Added it back in there 👍
dlaw4608
force-pushed
the
keystone_group
branch
from
b11149c to
bb24e02
Compare
- E2E tests included (except group-import-dependency) - API configured - Controller
dlaw4608
force-pushed
the
keystone_group
branch
from
bb24e02 to
03e01e8
Compare
3 participants
Add a controller for managing roles.
Closes issue: #582