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

Resource for GitLab instance (application) settings

Open JanKoppe opened this issue 2 years ago • 23 comments

Feature Description

Hello,

I would like to be able to set the instance wide "Application" settings from Terraform, when provisioning a new GitLab instance with Terraform. Right now, there's no way to do that with this provider, as far as I can see.

I'm specifically talking about these settings that apply to self-hosted instances: https://docs.gitlab.com/ee/api/settings.html

I think it would fit very well into this provider and provide huge value. Currently, I'm trying to do this using null_resources and manual invocations of curl, which is quite buggy.

Do you agree that this should be placed in this provider? If so, I'd be willing to work on it and see if I can give an implementation for such a resource.

Do you want to implement this?

  • [X] I would like to implement this myself 👷

JanKoppe avatar Mar 14 '22 14:03 JanKoppe

Do you agree that this should be placed in this provider? If so, I'd be willing to work on it and see if I can give an implementation for such a resource.

Yes, I think it would be nice.

However, I'm a little bit concerned that there is no actual create - as the resource can only exist once and kinda already exist outside of terraform. Thus, we need some dummy id and call the update API on create, too. We don't have such a resource yet - and this will most likely also be the only one ^^.

Feel free to take a stab at it and I'll be happy to review!

timofurrer avatar Mar 14 '22 15:03 timofurrer

However, I'm a little bit concerned that there is no actual create - as the resource can only exist once and kinda already exist outside of terraform. Thus, we need some dummy id and call the update API on create, too. We don't have such a resource yet - and this will most likely also be the only one ^^.

Yes, that's quite an anti-pattern to what you're usually doing in Terraform. But I think there will be a solution possible - easiest way probably would be to generate an artificial ID out of connection parameters, or something like that.

Feel free to take a stab at it and I'll be happy to review!

Great, will do so! Let's see if I can contribute a bit here. :)

JanKoppe avatar Mar 14 '22 16:03 JanKoppe

Sounds good. If you need any help implementing this, just ping me on the Discord server ;)

timofurrer avatar Mar 14 '22 17:03 timofurrer

Short update: Progress is ongoing, datasource is coming along, but the underlying go-gitlab library needs a few changes as well, lots of API changes are not reflected in that lib yet.

Also, I'm struggling a bit with the difficulties of the settings not being a real resource - especially what to do on a "delete", or when a parameter is being removed.

My approach so far is to keep this very simple and analog to the GitLab API design with just a single resource type, that has parameters for all the possible settings options.

If one destroys that resource, should we reset all settings to their defaults, as documented in the GitLab API docs? This feels a bit wrong, because it can have some unexpected side-effects for the user.

Another approach could be to reset just the settings that were defined on that resource, but I'm unsure if accessing that information is possible. Not experienced enough yet for that question, would appreciate any pointers in that direction.

JanKoppe avatar Mar 15 '22 20:03 JanKoppe

Short update: Progress is ongoing, datasource is coming along, but the underlying go-gitlab library needs a few changes as well, lots of API changes are not reflected in that lib yet.

Regarding the data source - I think it makes sense to create a dedicated PR for that.

Also, I'm struggling a bit with the difficulties of the settings not being a real resource - especially what to do on a "delete", or when a parameter is being removed.

Yes, I was also thinking about this and I'm kinda torn ... I see the need for having this as a resource, but at the same time, it isn't an "actual resource" w.r.t. how terraform defines a resource.

My approach so far is to keep this very simple and analog to the GitLab API design with just a single resource type, that has parameters for all the possible settings options.

👍

If one destroys that resource, should we reset all settings to their defaults, as documented in the GitLab API docs? This feels a bit wrong, because it can have some unexpected side-effects for the user.

Another approach could be to reset just the settings that were defined on that resource, but I'm unsure if accessing that information is possible. Not experienced enough yet for that question, would appreciate any pointers in that direction.

Not sure tbh. ... So, when "something" is managed with a tf resource, it's owned by the resource and it shouldn't change outside of tf. I could think of that when the resource is first put to state, we could read the settings and treat them as the values we set during remove. However, it's still a "hack" ... Maybe this will also be better fit into a provisioner ... (just thinking out loud)

timofurrer avatar Mar 16 '22 06:03 timofurrer

I personally never thought that this provider could be used for such purpose, but I'm curious to see how it turns out. Count on me to test and review the code.

willianpaixao avatar Mar 17 '22 08:03 willianpaixao

I think I'll focus on updating the underlying go-gitlab lib in the coming days, and try to finish the datasource after that first. This should nicely work with terraforms model and still be useful.

Let's see what approach will work for a possible resource after that work has been done. I'm not sure if there will ever be a "clean" solution, but maybe some more time to think about it will bring the least awful compromise. I guess such a resource will need a big disclaimer in the documentation.

JanKoppe avatar Mar 18 '22 09:03 JanKoppe

@JanKoppe maybe you can continue, I've already done the majority of missing fields: https://github.com/xanzy/go-gitlab/pull/1415

timofurrer avatar Mar 18 '22 15:03 timofurrer

@JanKoppe I am thinking the latest API response from GitLab is the source of truth and we expose those(pertaining to https://github.com/xanzy/go-gitlab/pull/1425#issuecomment-1087330553). As for documentation for this resource I recommend we pin versions in the description field for the resource depending on what version of GitLab you are using. What will end up happening is people will declare a second provider block specifically for this resource for their GitLab version to prevent breaks to older GitLab instances as we release new versions. We can document that pattern as well for this resource somewhere.

Shocktrooper avatar Apr 18 '22 18:04 Shocktrooper

@nagyv Is there an instance level API for integrations by chance I am missing or is the project level one the only Integrations API that currently exists. https://docs.gitlab.com/ee/api/integrations.html

Shocktrooper avatar Apr 18 '22 18:04 Shocktrooper

@nagyv Is there an instance level API for integrations by chance I am missing or is the project level one the only Integrations API that currently exists. https://docs.gitlab.com/ee/api/integrations.html

@Shocktrooper The project level is the only one, AFAIK.

nagyv avatar Apr 19 '22 06:04 nagyv

Ah yup found the issue here. https://gitlab.com/gitlab-org/gitlab/-/issues/335088

Shocktrooper avatar Apr 20 '22 04:04 Shocktrooper

@JanKoppe I am thinking the latest API response from GitLab is the source of truth and we expose those(pertaining to xanzy/go-gitlab#1425 (comment)). As for documentation for this resource I recommend we pin versions in the description field for the resource depending on what version of GitLab you are using. What will end up happening is people will declare a second provider block specifically for this resource for their GitLab version to prevent breaks to older GitLab instances as we release new versions. We can document that pattern as well for this resource somewhere.

That sounds like a reasonable approach to me. Will run with that.

JanKoppe avatar Apr 20 '22 12:04 JanKoppe

My golang is a bit rusty, but if there's anything I can do to help with this, I'd be happy to contribute.

I'm inclined to agree with the approach that would treat all the settings as a single resource. I think if someone removed a setting from the resource, then the sensible thing would be to revert it to the default or some kind of initial state value (i.e. null) if such a thing exists for the given setting. If that was undesirable for the user of the resource, they could always use a lifecycle rule to ignore a troublesome property.

tdg5 avatar May 18 '22 21:05 tdg5

Hey @tdg5, I'm currently quite busy and would only get to working on this again in a few weeks. If you want to, feel free to take a stab at it :) I was thinking if doing lots and lots of manual code in the provider is the right way to go (there are lots of arguments), or if it might make sense to do some fancy autogeneration instead, to reduce manual work (especially when doing updates). Just as an idea :)

JanKoppe avatar May 21 '22 06:05 JanKoppe

My golang is a bit rusty, but if there's anything I can do to help with this, I'd be happy to contribute.

@tdg5 In case you want to pick this up from @JanKoppe and need some help regarding Go or the provider, don't hesitate to stop by our Discord! We are happy to help and get you started 🤝

timofurrer avatar May 21 '22 08:05 timofurrer

@JanKoppe or @timofurrer Do you have any ideas for autogeneration? I was thinking 2 ways potentially

  1. Scan the official GitLab repo for instance settings and pull values from there. Downside might be that we include settings that might not be returned in an actual api call.
  2. Get the API settings of a demo GitLab instance via an API call and try to infer possible values from there. Downside is this might be pretty error prone for some settings but it should always reflect what is possible to get via an API call

Shocktrooper avatar May 22 '22 18:05 Shocktrooper

@Shocktrooper Yes, I've actually started to draft an OpenAPI spec for that endpoint (and contribute it to GitLab) in the hope that we can use it to generate the code required - at least for go-gitlab and then maybe also here.

It doesn't solve all issues we'll encounter, but at least it's "less" fragile. WDYT?

timofurrer avatar May 22 '22 18:05 timofurrer

@timofurrer Couple things with that potentially but I really like it and I do think it is less fragile.

  1. Will GitLab keep it up to date once added to keep it in sync with the actual API itself or is it potentially going to be forgotten and immediately become out of date from release to release.
  2. How much can we forward generate from the API spec you think reliably vs inferring from the code itself and what does the delta of work look like from the aerogeneration to final product.
  3. Per this thread are you also encapsulating the fields not returned in a normal API response if so we just need to think about how to handle those with auto code generation as I believe we want to only use fields returned in an API response and not undocumented ones

Shocktrooper avatar May 22 '22 21:05 Shocktrooper

@Shocktrooper I think we just have to try and move something forward. In any case I'm also fine just doing it by hand the first time to provide some value already. For (1) yes, that would be the goal. There already exist other OpenAPI specs.

timofurrer avatar May 23 '22 06:05 timofurrer

I think, if you manage to add automated tests to the OpenApi spec, then GitLab will keep it valid. I can not guarantee that it will be extended or there won't be breaking changes, but it will stay in the product in a working state.

The code that does the testing might be reused for the autogeneration. 🤔

Timo Furrer @.***> ezt írta (időpont: 2022. máj. 23., Hét 8:44):

@Shocktrooper https://github.com/Shocktrooper I think we just have to try and move something forward. In any case I'm also fine just doing it by hand the first time to provide some value already. For (1) yes, that would be the goal. There already exist other OpenAPI specs.

— Reply to this email directly, view it on GitHub https://github.com/gitlabhq/terraform-provider-gitlab/issues/957#issuecomment-1134247582, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAA65T6NVERNMBPNDIACQYDVLMSORANCNFSM5QVVPUHQ . You are receiving this because you were mentioned.Message ID: @.***>

nagyv avatar May 23 '22 06:05 nagyv

I've created a new gitlab_application_settings resource in #1201 in a semi-automated way.

I think it's something to unblock people, but it's also something that isn't perfect.

The most pressing points:

  • What does a destroy of this resource mean?
    • should we store an original state during create and restore to that during destroy?
      • would we do that for the entire resource (meaning all fields / attributes) or should we only touch the ones which were actually part of the resource.
  • would this destroy mechanism also apply when an attribute from the resource config is removed?
  • Should we support multiple gitlab_application_settings resources? e.g. so that different part of the application can be managed in different resource instances?
  • If an import would exist, what would it mean?

timofurrer avatar Aug 05 '22 13:08 timofurrer

I feel a few of these decisions are linked depending on the route we want to go. E.G. if we revert all settings on a destroy then we wouldn't be able to support multiple gitlab_application_settings resources as that would just result in deltas in the other terraform configs.

That aside as for supporting multiple gitlab_application_settings resources I don't see too many points because I imagine whomever uses this would want to control everything from a single resource now this could potentially change if someone wants to say configure the external_authorization_service_url setting separately as that value could be potentially more dynamic. The problem I see arising from something like this though is now you have potentially multiple points of control that you have to coordinate from different terraform configs.

Import: I would imagine we take all settings as they currently are and store them like you would in state for any other resource. This answer becomes complicated though if you start talking about only managing settings that are declared in the resource but how would you gate that on an import even. Not sure if imports become more complicated though if you talk about only managing the declared settings in the resource and leave the others alone that are undeclared, this I imagine depends on the answer question 1 subpoint 2

Shocktrooper avatar Aug 05 '22 19:08 Shocktrooper

We've implemented a first experimental version of a gitlab_application_settings resource which will be released with v.3.17.0. We'd appreciate feedback!

timofurrer avatar Aug 18 '22 07:08 timofurrer

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]