terraform-provider-gitlab icon indicating copy to clipboard operation
terraform-provider-gitlab copied to clipboard

resource/gitlab_application_settings: New experimental resource

Open timofurrer opened this issue 1 year ago • 4 comments

This change set implements the new gitlab_application_settings resource which provides a way to change the GitLab Application Settings using the API at https://docs.gitlab.com/ee/api/settings.html#change-application-settings.

Even though I suggest that we can merge this first experimental version of the resource there are still quite some open questions for me.

I'd like to tackle them over at https://github.com/gitlabhq/terraform-provider-gitlab/issues/957 and give the community the change to take part in the discussion on how to move forward with this.

Refs: #957 #1187

timofurrer avatar Aug 05 '22 12:08 timofurrer

@PatrickRice-KSC can you do me a favor and check if you can reproduce the CI failure locally?

timofurrer avatar Aug 07 '22 15:08 timofurrer

Hey @timofurrer - for sure. I tried this out on both GitPod (Failed, see first image below), and on a local ubuntu 21.04 vagrant machine (Failed, see second image below)

GitPod: image

Local Vagrant: image

PatrickRice-KSC avatar Aug 08 '22 15:08 PatrickRice-KSC

@timofurrer As I was looking through the code I don't think I saw anything specific but does there need to be anything done specifically to handle the setting that only appear once someone has an EE license vs a CE

https://docs.gitlab.com/ee/api/settings.html#:~:text=Users%20on%20GitLab%20Premium%20or%20Ultimate%20may%20also%20see%20these%20parameters%3A

Shocktrooper avatar Aug 08 '22 19:08 Shocktrooper

@timofurrer As I was looking through the code I don't think I saw anything specific but does there need to be anything done specifically to handle the setting that only appear once someone has an EE license vs a CE

https://docs.gitlab.com/ee/api/settings.html#:~:text=Users%20on%20GitLab%20Premium%20or%20Ultimate%20may%20also%20see%20these%20parameters%3A

The fields which are unavailable because of the license are "just not working" - you may set the attribute in the config, but will get an upstream error message - you may also access the attribute, but it won't have a value in it.

I think for a first version that's good enough. Actually, we do the same in many resources - which is only not really optimal.

timofurrer avatar Aug 10 '22 12:08 timofurrer

@timofurrer | @Shocktrooper - Is this PR ready for review now? Happy to take a look yet today or tomorrow!

PatrickRice-KSC avatar Aug 13 '22 21:08 PatrickRice-KSC

@PatrickRice-KSC yes, it is :)

timofurrer avatar Aug 13 '22 21:08 timofurrer

@PatrickRice-KSC I think those are left in because of the auto generation script. We should probably tweak that to leave those out of anything. An ignorelist persay

Shocktrooper avatar Aug 14 '22 01:08 Shocktrooper

@PatrickRice-KSC I think those are left in because of the auto generation script. We should probably tweak that to leave those out of anything. An ignorelist persay

@Shocktrooper @PatrickRice-KSC Yes, that's exactly what happened here. Also the verbiage alignments and ending sentences with a dot etc are because they are not really aligned in the GitLab docs itself. I think I'll apply your suggestions @PatrickRice-KSC and remove the deprecated attributes - let's see how often that endpoint really changes and has impacts for us. Maybe I'll add an OpenAPI spec for that endpoint and see what we can do to have it up-to-date in GitLab ...

timofurrer avatar Aug 15 '22 12:08 timofurrer

@Shocktrooper | @timofurrer - whew, I'm glad most of that code was generated. I got about 1/3 of the way through the PR and thought you guys had written that by hand 😮‍💨 . That makes sense - thanks!

PatrickRice-KSC avatar Aug 16 '22 00:08 PatrickRice-KSC

@Shocktrooper @PatrickRice-KSC I've updated the resource and removed all deprecated fields and did some alignment on the attribute descriptions. However, it's far from perfect, but addressing everything is just too much effort given the size of the resource. I suggest that we experiment with it as-is and in the future increase alignment in attributes and testing - again, maybe we can even provide some OpenAPI specs or something upstream to easily generate this resource.

WDYT? 🏓

timofurrer avatar Aug 17 '22 14:08 timofurrer

@timofurrer - Makes sense to me, given it's experimental I think we can align to standards a bit more in a next push. I can go through and resolve threads and take another look at it here in a bit to merge.

PatrickRice-KSC avatar Aug 17 '22 14:08 PatrickRice-KSC

This functionality has been released in v3.17.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!

github-actions[bot] avatar Aug 24 '22 18:08 github-actions[bot]