Add role assignment resource #265
Conversation
|
Build succeeded.
|
gonzolino
force-pushed
the
role-assignment
branch
from
5fbc4f4
to
33ee6d8
Compare
|
Build succeeded.
|
There was a problem hiding this comment.
@gonzolino I apologize for the late reply on this -- I'm not sure how I lost track of it.
In general, this looks good and this will be a great addition to have :)
I've left some comments for review, let me know if you have any questions.
| "openstack_identity_role_assignment_v3.role_assignment_1", "role_id", role.Name), | ||
| ), | ||
| }, | ||
| resource.TestStep{ |
There was a problem hiding this comment.
Since all attributes are ForceNew, we really don't need to test updating.
There was a problem hiding this comment.
thats true, removed the update test
| projectID, userID, roleID := split[1], split[3], split[4] | ||
|
|
||
| foundProject, err := projects.Get(identityClient, projectID).Extract() | ||
| if err == nil { |
| return err | ||
| } | ||
| foundUser, err := users.Get(identityClient, userID).Extract() | ||
| if err == nil { |
| return err | ||
| } | ||
| foundRole, err := roles.Get(identityClient, roleID).Extract() | ||
| if err == nil { |
| Config: testAccIdentityV3RoleAssignment_update, | ||
| Check: resource.ComposeTestCheckFunc( | ||
| testAccCheckIdentityV3RoleAssignmentExists("openstack_identity_role_assignment_v3.role_assignment_1", &role, &user, &project2), | ||
| resource.TestCheckResourceAttr( |
There was a problem hiding this comment.
This needs to be resource.TestCheckResourcePtrAttr and the value to check needs to be &project.ID.
There was a problem hiding this comment.
Ok, changed that bit (and the other two resource.TestCheckResourceAttr calls. Somehow I confused name and ID here 😛
But one question: Is there any particular reason why I should use resource.TestCheckResourceAttrPtr("openstack_identity_role_assignment_v3.role_assignment_1", "project_id", &project.ID) instead of resource.TestCheckResourceAttr("openstack_identity_role_assignment_v3.role_assignment_1", "project_id", project.ID) (without the pointer)?
There was a problem hiding this comment.
The comment for the TestCheckResourceAttrPtr function explains it better than I can :)
It took me a while to find that function. I was trying to debug a test for an hour or two and wondering why the value was always "". To get around that, TestCheckResourceAttrPtr is used.
| split := strings.Split(rs.Primary.ID, "/") | ||
| projectID, userID, roleID := split[1], split[3], split[4] | ||
|
|
||
| foundProject, err := projects.Get(identityClient, projectID).Extract() |
There was a problem hiding this comment.
Instead of checking for each resource individually, would it make more sense to do a ListOpts and ensure there's an assigned result?
There was a problem hiding this comment.
Ok that makes sense.
I still need to work on that though, since the acceptance tests are currently failing for me with this.
| user_id = "demo" | ||
| project_id = "demo" | ||
| role_id = "admin" | ||
| } |
There was a problem hiding this comment.
We can do a more elaborate "full-stack" test:
resource "openstack_identity_project_v3" "project_1" {
name = "project_1"
}
resource "openstack_identity_user_v3" "user_1" {
name = "user_1"
default_project_id = "${openstack_identity_project_v3.project_1.id}"
}
resource "openstack_identity_role_v3" "role_1" {
name = "role_1"
}
resource "openstack_identity_role_v3" "role_2" {
name = "role_2"
}
resource "openstack_identity_role_assignment_v3" "role_assignment_1" {
user_id = "${openstack_identity_user_v3.user_1.id}"
project_id = "${openstack_identity_project_v3.project_1.id}"
role_id = "${openstack_identity_role_v3.role_1.id}"
}| DomainID: domainID, | ||
| GroupID: groupID, | ||
| ProjectID: projectID, | ||
| UserID: userID, |
There was a problem hiding this comment.
These items should probably be removed :)
| resource "openstack_identity_role_assignment_v3" "role_1" { | ||
| user_id = "admin" | ||
| domain_id = "default" | ||
| role_id = "admin" |
There was a problem hiding this comment.
Maybe use the "full-stack" test fixture to provide a more verbose example here.
gonzolino
force-pushed
the
role-assignment
branch
from
33ee6d8
to
478dd50
Compare
|
@jtopjian Thanks for your comments and sorry for the delay, I was busy with other stuff the last days. I updated my patches but there is still work left to do, since the acceptance tests are failing for me. I'll try to fix that tomorrow. |
|
Build failed.
|
|
@gonzolino Absolutely no problem at all -- I've been swamped with work myself. Thank you so much for the updates to this :) I think the following should get the test to work: p, err := projects.Get(identityClient, assignment.Scope.Project.ID).Extract()
if err != nil {
return fmt.Errorf("Project not found")
}
*project = *p
u, err := users.Get(identityClient, assignment.User.ID).Extract()
if err != nil {
return fmt.Errorf("User not found")
}
*user = *u
r, err := roles.Get(identityClient, assignment.Role.ID).Extract()
if err != nil {
return fmt.Errorf("Role not found")
}
*role = *r |
gonzolino
force-pushed
the
role-assignment
branch
from
478dd50
to
06659b8
Compare
|
Build failed.
|
|
recheck |
|
Build succeeded.
|
|
@gonzolino This looks good to me. Were there any other changes you wanted to make or any other questions? |
|
@jtopjian The PR is now complete. I added all the tests and docs that I wanted. But if you think something is still missing, I would of course update the PR :) |
|
Nope - this looks great! I was just double-checking :) Thank you very much for your work on this! |
Adds resource openstack_identity_role_assignment_v3.
See #248 for more information (this PR contains part of the content that was initially proposed there).
Tests & docs are still missing.