jdamata / terraform-provider-sonarqube

Terraform provider for managing Sonarqube configuration
GNU General Public License v3.0
62 stars 51 forks source link

Added field setting for sonarqube_project resource #193

Closed pnag90 closed 10 months ago

pnag90 commented 11 months ago

Updated resource

Changes/fixes

freeranger commented 10 months ago

Personally I am not mad on taking this approach, closely mirroring the current Sonar API (which is a poor API IMO) - we should be modelling things "the terraform way" and mapping our "api" into the Sonar one.

An example of this is the implementation of Quality Gates which have child conditions. The condition, via the Sonar API is created completely independently from the quality gate itself and it reuires a gateName parameter when creating the condition in order to link it to the correct Quality Gate.

The plugin implementation could have separate sonarqube_qualitygate and sonarqube_condition resources, mirroring the API - but this is not taking advantage of terraform's ability to model child resources and be a truer reflection of the underlying model.
As with the Sonar API, it would be suggesting that a condition could exist in isolation or at best, does not express the need for the Quality Gate to exist prior to creating the condition - which is simply not the case and Sonar are relying on your "special knowledge" of their API to ensure you create things in the correct order.

The actual plugin implementation is described here: https://github.com/jdamata/terraform-provider-sonarqube/blob/master/docs/resources/sonarqube_qualitygate.md - it correctly models the parent/child relationship, does not require the quality gate to be specified directly on the child condition resource, and should ensure that the conditions are destroyed if the quality gate is removed. The same could not be said of an implementation where the parent Quality Gate and the child condition were completely separate resources.

Similarly, I'm pretty sure you can't have a setting which is linked to a project that does not exist, so why would we model a resource in that way? Yes the Sonar API models it that way but as I said, I believe this is a poor API. It would be great if we could support both stand-alone settings as we do today AND parent-child relationships - e.g.:


resource "sonarqube_setting" "single_global_setting" {
  key   = "sonar.demo"
  value = "sonarqube@example.org"
}

as well as:

resource "sonarqube_project" "main" {
    name       = "SonarQube"
    project    = "my_project"
    visibility = "public" 

   "setting" {
      key       = "sonar.demo"
      value     = "sonarqube@project-specificexample.org"
   }
}

Something like that would mean we are not relying on any special knowledge / order of creation, would not require any validation that the "component" to which the setting is attached already exists and, perhaps most importantly, should ensure that resources are correctly destroyed if the project is removed.

pnag90 commented 10 months ago

Thanks for your detailed point-of-view @freeranger 👍 I've made changes trying to follow that mindset!

freeranger commented 10 months ago

Thanks for your detailed point-of-view @freeranger 👍 I've made changes trying to follow that mindset!

No problem, always happy to opine on something I have an interest in :)

Great that you are refactoring the PR to better model the relationships between the resources. I see you have done this for projects but I think you need to make the same changes (and add tests and documentation of course) to the portfolio resource too. We don't seem to have an application resource in the provider but it would apply there also if we had one...something for the person who adds applications to think about....

cheers!

pnag90 commented 10 months ago

I was only able to add the settings support to project resource. Even testing the API, the portfolio did not work. So for now, I am only submitting the change to projects. In future, if intended, the same logic methods can be shared between resources.

freeranger commented 10 months ago

I was only able to add the settings support to project resource. Even testing the API, the portfolio did not work.

FYI I am trying to find out from Sonar if this end point should actually work for portfolios and applications and get an example of each type of setting if it does. If it does not in fact work for these then hopefully they will update their inaccurate documentation!

jdamata commented 10 months ago

I was only able to add the settings support to project resource. Even testing the API, the portfolio did not work. So for now, I am only submitting the change to projects. In future, if intended, the same logic methods can be shared between resources.

Thanks for your work on this! I'm returning home from vacation Thursday. Will try to find time to review this when I'm back

freeranger commented 10 months ago

I was only able to add the settings support to project resource. Even testing the API, the portfolio did not work.

FYI I am trying to find out from Sonar if this end point should actually work for portfolios and applications and get an example of each type of setting if it does. If it does not in fact work for these then hopefully they will update their inaccurate documentation!

@pnag90 I finally got a response back from Sonar. Whilst the end point is mainly for use for projects, there are some cases where it is used elsewhere - sonar.governance.report.project.branch.frequency can be set for applications for example. And third party plugins can define settings at any of the three levels - applications, portfolios, projects.

They didn't provide a "built in" example for portfolios and said "For the Portfolios which have a similar setting a dedicated Web API is used. However we might change this and new or existing settings might take advantage of this Web API in the future for Portfolios and Applications" - I am going back to them again about that as it is not entirely clear to me - but at least you now have an example for applications which you could write a test to use. I have tested it locally with Postman: POST api/settings/set?component=MyApp&key=sonar.governance.report.project.branch.frequency&value=Daily and it does indeed work.

freeranger commented 10 months ago

FYI Sonar have come back to me again on the portfolios case - they confirmed that the endpoint is not used for them at the moment (so the documentation is incorrect). An example of another end point which is being used for those settings would be api/governance_reports/update_frequency

One could either decide to support portfolios in the same way as applications and projects, but behind the scenes make this call or indeed just ignore portfolios for now.... Either way, I will ask Sonar to correct the documentation.

Sameer2307 commented 4 months ago

How can I add project visibility to multiple groups Looks like it accepts a string only, Can someone help me thanks