-
Notifications
You must be signed in to change notification settings - Fork 289
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
Add support to manage access packages in identitygovernance #903
Conversation
thank you for your work, we're eagerly waiting for this PR to be merged 🙏 |
Nice one! Feel free to take anything from #612 - encountered some of the similar issues you've mentioned. I did a fair amount of tests and seemed alright but touch and go. Good luck actually getting your SDK changes sorted as any changes to identitygov have been in limbo for a long time as I'm sure you've seen from my blockers on my PR manicminer/hamilton#156 |
Wrote a module to manage these resource this week, fixed a few issues. However the following resources are completely not working due to the issues mentioned in manicminer/hamilton#187, one of them is recently introduced, once these are fixed, the following resources should work as expected.
I guess it's ready to be merged from my view. |
@manicminer @katbyte able to take a look at this? Thanks. |
@stanleyz Thanks for the contribution, and for your patience! I will make some time to review this in the next week or so. |
@stanleyz As this PR has more features than my older draft PR I'll drop my PR for yours and and update the issue tracker :) - As mentioned in my issue did not originally set out to do the full set as wasn't sure if a dummy delete would be accepted, or whether to implement something that recreates the resource, while hacks and workarounds could be done on the user end its not ideal, but having this in would be better than nothing. Its pretty rare to want to delete sub level resource roles anyway unless you make a mistake, as you'd would just end up deleting the whole package. |
Thanks @kaovd , pretty much same reasoning as my thinking. Additionally although I haven't tested yet regarding force recreation of new access packages, I am not sure whether that will succeed in case there are already assignments on that policy. |
oh, we're so ready to start using this one 😁 thank you @stanleyz @manicminer 🙏 |
Hey @manicminer, any updates on when this might be reviewed? |
@manicminer any update on this ? |
Am I the correct assumption that this PR builds upon the BETA graph endpoint and therefore does not have an option to enable "autoassignments" ? I've also not seen any options to handle "auto-assignments" via Graph API on Beta and the attribute I've managed to create those dynamic rules with the V1.0 API and they seem to have a few differences. With the All the other functionalities are working as expected and are awesome! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
small typo in flattenApprovalSettings
internal/services/identitygovernance/access_package_assignment_policy_resource.go
Outdated
Show resolved
Hide resolved
Haha, it has been some time that I have forgotten some details, but we only use Beta API for accessPackageClient as shown below, the reason of using Beta API for access package is explained here.
when you say |
Thanks for the explanation and the link. I mean As the schema of V1.0 and Beta are quite different, I also don't think that it's applicable to just change the client. |
internal/services/identitygovernance/access_package_assignment_policy_resource.go
Outdated
Show resolved
Hide resolved
Yes, this requires running Terraform destroy twice to delete the association because there is MS API to delete the association between the access package and group, so it relies on the access package to deleted first, then the group can be removed from a catalog.
This is a bit weird, I never had this issue in my use, might need take a look further with detailed logs. |
Right, it doesn't seem the upstream SDK supports this yet, see below code.
As said, the policy uses v1.0 API, only the access package resource uses beta. |
Pretty please 😊 |
As commenting on the thread emails everyone who is subscribed to the topic can we keep comments on topic rather than asking for status updates? It's a pain having to wait but asking won't make it come quicker (Putting a thumbs up on the PR itself is a good way to show your interest in the feature) |
Some typos in the doc, add suggestions from the review. Co-authored-by: ccadruvi <c.cadruvi@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @stanleyz for working on and submitting this - this addition is highly appreciated especially with working out how best to map these into Terraform resources. I'm happy with the design and the high quality of the implementation meant that I did not have to rework anything.
Apologies to all subscribers for the delay in getting to this, I had hoped to get this in awhile back. I've worked on this the last couple days to get it over the line, the tests look good and it's now ready to go.
Many thanks @kaovd, @mazlumtoprak, @tomasmota, @arsatiki, @ccadruvi, @cedrox for testing and reviewing. And @aleksei-aikashev for the encouragement.
This functionality has been released in v2.37.0 of the Terraform Provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading. For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template. Thank you! |
Resources added:
Data resource added:
Issues:
Minor:
Closes: #218
Closes: #547