-
Notifications
You must be signed in to change notification settings - Fork 319
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
Update managed_license resources to deprecate existing values and add GitLab 15+ values #952
Update managed_license resources to deprecate existing values and add GitLab 15+ values #952
Conversation
Note - I chose to leave the existing values under a deprecated test, which will automatically stop running once we update to GitLab 15+. This ensures that we don't accidentally regress in the meantime. |
Hm, jumped the gun a bit on this PR I think. Was just testing this on GitLab.com, and it appears the new values are not accepted yet, so folks will have a "hard" cut-over in version 15. That's a bit annoying. I'll likely close this PR and then re-push it post 15.0 (May). |
And nevermind once again. Arg. I was testing with an older project and having issues with my tests. New approval statuses are implemented, so this PR is valid. |
Since I don't have rights to re-open a PR, can someone reopen this so that I don't duplicate PRs in the repo and clutter it up? This should be ready for testing. |
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.
looks good, except a minor comment :)
So weird thing about the EE failure - this is happening because the "Read" API always returns the old deprecated results no matter which value is passed in. You pass in "allowed", you get back "approved" with a success. You pass in "denied", you get back "blacklisted" with a success. You can then update them interchangeably, but you'll always get back the old value. I see a couple different ways of fixing this and I'm interested in your guys opinions:
It seems like option 1 may be the "best" path, and it's relatively easy to do. We'd have to leave it in the codebase for a while though, since there may be users who use the tfprovider on earlier gitlab self-hosted versions, so we'd have to pre-emptively deprecate it for 4.0 I think. |
@PatrickRice-KSC that's a pity -.^ Do you know if there exists an upstream issue for that? Actually there is another way (and probably the cleanest one), which is to use a However, I'm not sure what would happen if you'd switch from the deprecated value to the new value, if you can make it so that the API is still being called ... I suggest you play around with it a little bit and see if we can use it :) |
As far as I can tell, this is actually how they intended this to work. It's noted this way in the deprecation notice after I went back and read it a second time (after observing the behavior): https://docs.gitlab.com/ee/update/deprecations.html#legacy-approval-status-names-from-license-compliance-api
Sweet, I'll take a look. I'll definitely need to play around with that a bit since I've not needed to use it before. |
I think this will be pretty simple. I'm going to assume that in 15.0 that GitLab will automatically migrate everything from "allowed" to "approved" and from "blacklisted" to "denied", just because there is no way they'll make everyone re-submit all their licenses. So if I set "allowed == approved", then it'll suppress the diff the second time around (and make the tests pass). The only odd thing here is that if a user has a config that was applied pre-15.0, it'll pass until they attempt to add a new license, then that new license will fail with an invalid value and force them to update the verbiage to "approved" for all licenses which will ignore applying for all old licenses, and just apply for the new licenses. Thus, I don't think we need to make it call the API even if they're attempting to update from "allowed -> approved". Testing and committing. |
Once we're done with reviews, let me know please and I'll squash the commits. I'm not a fan of 9+ commits on a PR 😄 |
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.
LGTM 🎉
You can do the squashing and once tests passed I'll merge :)
Nice work 🌮
Update documentation, add SkipFunc. Map values. Update SkipFunc implementation, map test statuses appropriately. Update internal/provider/helper_test.go Co-authored-by: Timo Furrer <[email protected]> Add DiffFunc for the managed license status Fix bad log on unit test Update internal/provider/resource_gitlab_managed_license.go Update internal/provider/resource_gitlab_managed_license.go Update documentation for the approval_status field
Done - I rebased as long as I was in there as well, so we should be good when the tests finish. |
This functionality has been released in v3.13.0 of the Terraform GitLab 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. Thank you! |
Description
Closes #951
PR Checklist Acknowledgement
//lintignore
comments were copied from existing code. (Linter rules are meant to be enforced on new code.)