Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] Extend identity resources & data sources #248

Closed

Conversation

gonzolino
Copy link
Contributor

This PR adds resources openstack_identity_role_v3 and openstack_identity_user_role_v3 and data sources openstack_identity_project_v3, openstack_identity_role_v3 and openstack_identity_user_v3 in an effort to improve support for the identity service in openstack.

It is still a WIP. Currently there are tests missing and a openstack_identity_group_role_v3 resource (alternatively, I am thinking about removing openstack_identity_user_role_v3 and replacing it with a resource openstack_identity_role_assignment_v3 that handles role assignments for users and groups).

Also, I am hoping for some feedback from maintainers, since this is my first PR on a terraform provider and I suspect that I might have made some rookie mistakes in the code ;)

The openstack_identity_user_role_v3 resource should fix #237.

@theopenlab-ci
Copy link

theopenlab-ci bot commented Feb 20, 2018

Build failed.

@jtopjian
Copy link
Contributor

jtopjian commented Feb 20, 2018

@gonzolino Thank you for your work on this.

I see that this is still marked as [wip] but wanted to touch base on a few things:

It might make more sense to break this up into a few PRs. This PR includes 5 different resources (3 data sources and 2 resources), which is a lot going on. I would be OK with including, say, the Role data source and resource in a PR. So perhaps:

  1. Role data source and resource
  2. Project data source
  3. User data source
  4. User Role resource

Additionally, would you be able to supply acceptance tests for these? If not, that's understandable, but I'll need to create some before they're merged.

And finally, we'll want to include docs with these resources, too.

Again, thank you for working on this. These are much needed items and it's really appreciated. Please let me know if you have any questions or need any help.

edit: Sorry, a few more items (too early in the morning):

openstack_identity_role_assignment_v3

I agree with this - this is a much better name for the resource.

Also, the test failure can be ignored. The failing test in question sometimes fails and it has nothing to do with the code.

@gonzolino
Copy link
Contributor Author

@jtopjian , thanks for your comments.
In short: I agree to all of them ;) The reason I pushed the PR as WIP was to get early feedback.
My plan is to start by replacing openstack_identity_user_role_v3 with openstack_identity_role_assignment_v3, splitting up the PR as you suggested and then add tests & docs to the different PRs.
As soon as I start with the tests &docs some help would be nice, since I believe this could get quite work-intensive...

@jtopjian
Copy link
Contributor

Sounds like a good plan. 😄

I did a cursory review of the code and it all looks good to me. I can do a more detailed review once the PRs start getting split up.

As soon as I start with the tests &docs some help would be nice, since I believe this could get quite work-intensive...

Sure, no problem at all and very understandable.

tests+docs is going to double the amount of work, so absolutely let me know if I can help.

Docs actually aren't really bad. All docs are located in the website directory of this repo. Data sources go under d and resources under r. Then you'll want to update the sidebar at website/openstack.erb.

For acceptance tests, you might be able to use an existing test file as a template. At a minimum, the resource_openstack_identity_project_v3_test.go (and user) file will be helpful. For a data source, the data_source_openstack_compute_flavor_v2_test.go is pretty simple and might be helpful (even though it's not related to the Identity service).

But I can definitely assist with this. Acceptance tests can sometimes be a lot more complicated than the resource itself, so please let me know if you run into issues before you spend a number of hours on it 😄

@gonzolino
Copy link
Contributor Author

Short update: I replaced openstack_identity_user_role_v3 with openstack_identity_role_assignment_v3. I will now continue to break this PR into smaller PRs as suggested (this PR can later be closed as soon as all code has been moved to other PRs).

@theopenlab-ci
Copy link

theopenlab-ci bot commented Feb 25, 2018

Build succeeded.

@gonzolino
Copy link
Contributor Author

I am closing this PR, since all changes proposed here are now included in PRs #250, #251, #252 and #265.

@gonzolino gonzolino closed this Mar 13, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enhancement: add ability to assign user to role on a project
2 participants