jdamata / terraform-provider-sonarqube

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

Portfolio manual selection #211

Closed felixlut closed 11 months ago

felixlut commented 11 months ago

This PR introduces:

Side note, @jdamata you're a fucking hero for dealing with the SonarQube API as much as you have ❤️

Using SonarQube at work wouldn't be possible without this. The fact that SonarQube themselves isn't maintaining a provider is beyond me

jdamata commented 11 months ago

This PR introduces:

  • Manually adding projects to Portfolios
  • These projects can include specific branches, or if left empty will default to the MAIN BRANCH of the project

Side note, @jdamata you're a fucking hero for dealing with the SonarQube API as much as you have ❤️

Using SonarQube at work wouldn't be possible without this. The fact that SonarQube themselves isn't maintaining a provider is beyond me

Thanks for the kind words and the contribution!!! Would be pretty cool if SonarSource wanted to fork and maintain a official provider.

felixlut commented 11 months ago

Continuation of comments here.

Hmm, so this seems to break existing Portfolios. Having a MANUAL portfolio created via Terraform before this PR, which has been populated manually via the UI, will result in an empty portfolio if not adhered to the changes here: image

I'm not sure what the way forward here should be, I can see the following options:

  1. Release this as a breaking change. The issue here is that Terraform plan still works, and silently just empties out any Portfolio that exists
  2. Introduce a use_selected_projects field which defaults to false. This boolean would need to be explicitly set for selected_projects to be used, and thereby guarantee that it will break anything. Another bonus with this is that it actually is possible to create a MANUAL portfolio via Terraform and populating it via the UI (why you would do that is beyond me). The code as it is now will result in that not being possible without this option.
  3. Creating a separate resource called sonarqube_portfolio_project, which essentially does what the nested selected_projects block now. You'd create one sonarqube_portfolio_project for each project to add to the portfolio. The issue I see with this is that this resource would depend on that the portfolio would have selection_mode set to MANUAL. I'm not sure how you'd handle changes in selection_mode. Would you simply have sonarqube_portfolio_project resources be deleted from the state if it changes? I'm usually in favor of "sub-resources" as opposed to bundling everything into a big one, but this one I'm not sure of.
  4. Make selected_projects mandatory if the portfolio is in MANUAL MODE. This would avoid the silent problem of option 1, but introduce that manual portfolios need to define their projects in Terraform fully. I'd imagine that is the way most would handle manually added projects, but fuck do I know 🤷

@jdamata I would appreciate feedback here. I'm leaning towards option 2, or maybe 1, but I'm not entirely sure.

jdamata commented 11 months ago

Continuation of comments here.

Hmm, so this seems to break existing Portfolios. Having a MANUAL portfolio created via Terraform before this PR, which has been populated manually via the UI, will result in an empty portfolio if not adhered to the changes here: image

I'm not sure what the way forward here should be, I can see the following options:

  1. Release this as a breaking change. The issue here is that Terraform plan still works, and silently just empties out any Portfolio that exists
  2. Introduce a use_selected_projects field which defaults to false. This boolean would need to be explicitly set for selected_projects to be used, and thereby guarantee that it will break anything. Another bonus with this is that it actually is possible to create a MANUAL portfolio via Terraform and populating it via the UI (why you would do that is beyond me). The code as it is now will result in that not being possible without this option.
  3. Creating a separate resource called sonarqube_portfolio_project, which essentially does what the nested selected_projects block now. You'd create one sonarqube_portfolio_project for each project to add to the portfolio. The issue I see with this is that this resource would depend on that the portfolio would have selection_mode set to MANUAL. I'm not sure how you'd handle changes in selection_mode. Would you simply have sonarqube_portfolio_project resources be deleted from the state if it changes? I'm usually in favor of "sub-resources" as opposed to bundling everything into a big one, but this one I'm not sure of.
  4. Make selected_projects mandatory if the portfolio is in MANUAL MODE. This would avoid the silent problem of option 1, but introduce that manual portfolios need to define their projects in Terraform fully. I'd imagine that is the way most would handle manually added projects, but fuck do I know 🤷

@jdamata I would appreciate feedback here. I'm leaning towards option 2, or maybe 1, but I'm not entirely sure.

I think i am leaning to option 4. Silently failing/wiping out manual configuration sounds pretty harsh for a breaking change. If there are folks that don't like to fully define their project in terraform, we can revise this later in a future feature request. Otherwise option 2 sounds ok to me as well.

felixlut commented 11 months ago

Continuation of comments here. Hmm, so this seems to break existing Portfolios. Having a MANUAL portfolio created via Terraform before this PR, which has been populated manually via the UI, will result in an empty portfolio if not adhered to the changes here: image I'm not sure what the way forward here should be, I can see the following options:

  1. Release this as a breaking change. The issue here is that Terraform plan still works, and silently just empties out any Portfolio that exists
  2. Introduce a use_selected_projects field which defaults to false. This boolean would need to be explicitly set for selected_projects to be used, and thereby guarantee that it will break anything. Another bonus with this is that it actually is possible to create a MANUAL portfolio via Terraform and populating it via the UI (why you would do that is beyond me). The code as it is now will result in that not being possible without this option.
  3. Creating a separate resource called sonarqube_portfolio_project, which essentially does what the nested selected_projects block now. You'd create one sonarqube_portfolio_project for each project to add to the portfolio. The issue I see with this is that this resource would depend on that the portfolio would have selection_mode set to MANUAL. I'm not sure how you'd handle changes in selection_mode. Would you simply have sonarqube_portfolio_project resources be deleted from the state if it changes? I'm usually in favor of "sub-resources" as opposed to bundling everything into a big one, but this one I'm not sure of.
  4. Make selected_projects mandatory if the portfolio is in MANUAL MODE. This would avoid the silent problem of option 1, but introduce that manual portfolios need to define their projects in Terraform fully. I'd imagine that is the way most would handle manually added projects, but fuck do I know 🤷

@jdamata I would appreciate feedback here. I'm leaning towards option 2, or maybe 1, but I'm not entirely sure.

I think i am leaning to option 4. Silently failing/wiping out manual configuration sounds pretty harsh for a breaking change. If there are folks that don't like to fully define their project in terraform, we can revise this later in a future feature request. Otherwise option 2 sounds ok to me as well.

Option 4 it is! Since we can't really tell how the provider is used in the wild, I think this is a reasonable compromise. The work needed for 4 is minimal, and we are only potentially breaking something. We don't actually know that this is a usecase that someone has, we can only speculate. So I agree with your assessment that a future feature request can rectify it if that is indeed the case

felixlut commented 11 months ago

@jdamata I've learnt something new. Terraform has a built in system for validating a schema after a plan is completed (docs here). My changes since your last review moves the validatePortfolioResource() validation inside the schema declaration itself. If I understand the documentation correctly, essentially what happens is that terraform plan make a call to resourceSonarqubePortfolioRead, and then subsequently validates the changes the plan would make. This leads to the above issue (Terraform created MANUAL portfolio with manually added projects) failing with the following error when no projects are set in the resource:

resource "sonarqube_portfolio" "main" {
    key         = "test-tf-portfolo"
    name        = "test-tf-portfolo"
    description = "test-tf-portfolo"
    selection_mode = "MANUAL"
    # selected_projects {
    #   project_key = "my-cool-project"
    # }
}

image

jdamata commented 11 months ago

@felixlut LGTM. Are we good to merge this?

felixlut commented 11 months ago

Yes! And just so you don't forget, it's a breaking change

felixlut commented 11 months ago

@jdamata can we get a new release with this change? I have a Jira ticket with my name on it I wanna close 😅