feat(jaas-role): add jaas' role support #648
Conversation
SimoneDutto
changed the title
SimoneDutto
force-pushed
the
JUJU-7235/jaas-role
branch
4 times, most recently
from
0f3519c to
4afaf97
Compare
SimoneDutto
force-pushed
the
JUJU-7235/jaas-role
branch
from
4afaf97 to
4184607
Compare
|
/build |
|
|
||
| ### Required | ||
|
|
||
| - `access` (String) Level of access to grant. Changing this value will replace the Terraform resource. Valid access levels are described at https://canonical-jaas-documentation.readthedocs-hosted.com/en/latest/reference/authorisation_model/#valid-relations |
There was a problem hiding this comment.
the linked page needs to be updated with valid access levels for roles
There was a problem hiding this comment.
nice catch, i'll propose a PR to change that page soon
SimoneDutto
force-pushed
the
JUJU-7235/jaas-role
branch
from
4184607 to
69af696
Compare
There was a problem hiding this comment.
The commit messages and pr description must include detail about why the change is being made, not solely what the change is. Be kind to your future self.
The PR description is missing how to test this change.
Was there agreement that we were skipping the process requiring a schema change GitHub issue to be filed and approved before the PR? There are 2 new resources, 1 new data source, and multiple schema changes which should have gone thru this process.
My review is not completed, but passing along the current thoughts. The group resource not using roles needs to be addressed.
| // ReadRoleByUUID attempts to read a role that matches the provided UUID. | ||
| func (jc *jaasClient) ReadRoleByUUID(uuid string) (*JaasRole, error) { | ||
| return jc.readRole(¶ms.GetRoleRequest{UUID: uuid}) | ||
| } | ||
|
|
||
| // ReadRoleByName attempts to read a role that matches the provided name. | ||
| func (jc *jaasClient) ReadRoleByName(name string) (*JaasRole, error) { | ||
| return jc.readRole(¶ms.GetRoleRequest{Name: name}) | ||
| } | ||
|
|
There was a problem hiding this comment.
These two methods are implementation details. Which the caller shouldn't have to care about. It's cleaner at a package level to have a ReadRole method, then internally decide if it's a UUID or string.
| Required: true, | ||
| }, | ||
| "uuid": schema.StringAttribute{ | ||
| Description: "The UUID of the role.", |
There was a problem hiding this comment.
suggestion: add a sentence describing how the uuid could be used.
| @@ -121,6 +123,14 @@ func (r *genericJAASAccessResource) partialAccessSchema() map[string]schema.Attr | |||
| setvalidator.ValueStringsAre(ValidatorMatchString(jimmnames.IsValidGroupId, "group ID must be valid")), | |||
| }, | |||
| }, | |||
| "roles": schema.SetAttribute{ | |||
| Description: "List of roles to grant access.", | |||
There was a problem hiding this comment.
todo: update description to include how the roles are specified, name? uuid?
| // set roles to empty set because we don't support assign roles to group | ||
| emptyRoleSet, diagEmptySet := types.SetValue(types.StringType, nil) | ||
| diag.Append(diagEmptySet...) |
There was a problem hiding this comment.
A different approach is needed if the jaas access group doesn't have roles. If a user does put roles into the jaas access group resource, when you write the data back, terraform cli will error as the values are not the same. I'm surprised testing hasn't revealed a problem here, though the tests haven't been updated for this resource.
|
@hmlanigan I didn't know we required a process to add a new resource, would you like me to open these issues before addressing the comments here? |
I will defer to @alesstimec on this one. |
|
Role data source pr: #654 |
|
Change to resource access schema: #655 |
alesstimec
added
the
state/codefreeze
Description
In this pr we add the support for jaas' roles to the terraform provider.
This pr is very similar to the one for groups. (#570 and #602).
It required upgrading the
jimm-go-sdk.Type of change