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

resource/gitlab_application_settings: New experimental resource #1201

Merged

Conversation

timofurrer
Copy link
Member

@timofurrer timofurrer commented Aug 5, 2022

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 timofurrer added blocked We are currently blocked to implement this go-gitlab Is related to go-gitlab. Maybe be combined with other labels, like `blocked`. labels Aug 5, 2022
@timofurrer timofurrer self-assigned this Aug 5, 2022
@timofurrer timofurrer force-pushed the feature/application-settings branch 6 times, most recently from 01bc29f to 8f324cf Compare August 7, 2022 14:46
@timofurrer timofurrer removed blocked We are currently blocked to implement this go-gitlab Is related to go-gitlab. Maybe be combined with other labels, like `blocked`. labels Aug 7, 2022
@timofurrer
Copy link
Member Author

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

@PatrickRice-KSC
Copy link
Collaborator

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

@Shocktrooper
Copy link
Collaborator

@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

@timofurrer
Copy link
Member Author

@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.

@PatrickRice-KSC
Copy link
Collaborator

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

@timofurrer
Copy link
Member Author

@PatrickRice-KSC yes, it is :)

Copy link
Collaborator

@PatrickRice-KSC PatrickRice-KSC left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@timofurrer That is a huge PR, I'll probably need to take a second pass through it after you changes just to double check all the sets and everything in the schema file; my brain was getting a bit sketchy partway through reviewing it!

Lots of comments, but they really boil down to 2 topics: Do we really want to support already deprecated attributes that are almost a year deprecated in some cases, and just fixing minor grammar in descriptions.

Thanks!

internal/provider/schema_gitlab_application_settings.go Outdated Show resolved Hide resolved
internal/provider/schema_gitlab_application_settings.go Outdated Show resolved Hide resolved
internal/provider/schema_gitlab_application_settings.go Outdated Show resolved Hide resolved
internal/provider/schema_gitlab_application_settings.go Outdated Show resolved Hide resolved
internal/provider/schema_gitlab_application_settings.go Outdated Show resolved Hide resolved
@Shocktrooper
Copy link
Collaborator

@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

@timofurrer
Copy link
Member Author

@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 ...

@PatrickRice-KSC
Copy link
Collaborator

@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!

@timofurrer
Copy link
Member Author

@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? 🏓

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
@PatrickRice-KSC
Copy link
Collaborator

@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.

@github-actions
Copy link

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 github-actions bot locked and limited conversation to collaborators Nov 9, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
3 participants