Portfolio manual selection #211

felixlut · 2023-10-12T16:51:45Z

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

This reverts commit 8f591da.

This reverts commit 5ae9903.

jdamata · 2023-10-13T19:12:05Z

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.

jdamata · 2023-10-13T00:20:21Z

sonarqube/resource_sonarqube_portfolio.go

 	}
 	return nil
 }

 // Validate the regexp and tag fields if the corresponding selection_mode is chosen
 func validatePortfolioResource(d *schema.ResourceData) error {
 	switch selectionMode := d.Get("selection_mode"); selectionMode {
-	case "NONE", "MANUAL", "REST":
+	// TODO: Validate MANUAL properly


Can you be more descriptive on what we need to validate here. So that in the future we know what should be checked.

Ops I forgot to remove the comment. My idea when I wrote it was that if MANUAL mode was selected, then there should be a validation that selected_projects should have at least one value in it, but I later realized that should not be done.

Firstly because you can just create a MANUAL portfolio and not populate it in the UI. The same should be possible in the provider. But also, if anyone today is using the provider to define MANUAL portfolios, and then add projects to it manually, then using the provider after this goes through would crash.

Actually, that's a test case I didn't cover. What will happen to MANUAL portfolios with projects added outside of terraform. I think the behaviour should be that a bump in version of this provider should not change them at all, but I'll have to test it. I'll do that today or tomorrow

felixlut · 2023-10-15T14:45:42Z

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:

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

Release this as a breaking change. The issue here is that Terraform plan still works, and silently just empties out any Portfolio that exists
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.
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.
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 · 2023-10-17T15:32:58Z

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:

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

Release this as a breaking change. The issue here is that Terraform plan still works, and silently just empties out any Portfolio that exists

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.

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.

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 · 2023-10-17T17:02:15Z

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:
I'm not sure what the way forward here should be, I can see the following options:

Release this as a breaking change. The issue here is that Terraform plan still works, and silently just empties out any Portfolio that exists

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.

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.

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 · 2023-10-17T18:45:24Z

@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"
    # }
}

sonarqube/resource_sonarqube_portfolio_test.go

jdamata · 2023-10-17T21:48:52Z

@felixlut LGTM. Are we good to merge this?

felixlut · 2023-10-18T05:06:47Z

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

felixlut · 2023-10-19T13:12:25Z

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

felixlut and others added 30 commits September 4, 2023 22:12

add constants for the selection modes

61ca933

fix linting issue

1a401b2

Merge branch 'master' into portfolio-manual-selection

d769415

Merge branch 'jdamata:master' into portfolio-manual-selection

0469554

add projects to the terraform schema

ab65a64

Update resource_sonarqube_portfolio.go

c786495

add support for adding a sonarqube project manually to portfolio

5fda36f

impletement updating of sonarqube portfolio projects

00fa4d7

implement adding project branches to portfolios

015b7ad

add back 'key'

1ab4bd0

add initial test

3f9347e

Update resource_sonarqube_portfolio_test.go

03c52d2

Update resource_sonarqube_portfolio_test.go

51d1b09

add proper testcases

39cb4e3

convert selected_branches to set since

761e6ea

convert selected_project to set

9ec81a5

run tests in ci because local computer is struggling...

cb08c90

run nested test

383b304

this should fail

8f591da

Revert "this should fail"

d04c117

This reverts commit 8f591da.

this should fail

5ae9903

Revert "this should fail"

6fc9b8b

This reverts commit 5ae9903.

update structure of tests

e3dff5d

update project keys to proper values

6ee55bb

im stupid

f20aa42

add import test case

5d7b2e0

add test for removing selected_branches

c57693d

add test for adding and removing selected_projects

2bb393c

update name of test

927ab8c

fix test

2bd0122

felixlut and others added 2 commits October 12, 2023 18:31

Update resource_sonarqube_portfolio.go

c059374

remove changed variable

6a1b52a

felixlut marked this pull request as draft October 12, 2023 16:52

add docs

57fab8b

felixlut marked this pull request as ready for review October 12, 2023 17:01

jdamata self-requested a review October 13, 2023 19:10

jdamata approved these changes Oct 13, 2023

View reviewed changes

felixlut and others added 2 commits October 15, 2023 16:51

add example usage of selected_projects in portfolio

d86b58c

remove 'projects' since I never use it anyways

ef72f2a

felixlut added 2 commits October 17, 2023 20:26

it works now i think

0496fef

move resource validation into post plan phase

afedc78

remove old testcase covered by the new ones

30c5e9f

felixlut commented Oct 17, 2023

View reviewed changes

sonarqube/resource_sonarqube_portfolio_test.go Show resolved Hide resolved

verify the imported resource

e18f261

jdamata merged commit c8754ed into jdamata:master Oct 18, 2023
11 checks passed

felixlut deleted the portfolio-manual-selection branch October 19, 2023 07:59

felixlut mentioned this pull request Oct 22, 2023

Fix ruleset bypass actors diff issues integrations/terraform-provider-github#1950

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Portfolio manual selection #211

Portfolio manual selection #211

felixlut commented Oct 12, 2023 •

edited

Loading

jdamata commented Oct 13, 2023

jdamata Oct 13, 2023

felixlut Oct 14, 2023

felixlut Oct 14, 2023

felixlut commented Oct 15, 2023 •

edited

Loading

jdamata commented Oct 17, 2023

felixlut commented Oct 17, 2023

felixlut commented Oct 17, 2023 •

edited

Loading

jdamata commented Oct 17, 2023

felixlut commented Oct 18, 2023

felixlut commented Oct 19, 2023 •

edited

Loading

Portfolio manual selection #211

Portfolio manual selection #211

Conversation

felixlut commented Oct 12, 2023 • edited Loading

jdamata commented Oct 13, 2023

jdamata Oct 13, 2023

Choose a reason for hiding this comment

felixlut Oct 14, 2023

Choose a reason for hiding this comment

felixlut Oct 14, 2023

Choose a reason for hiding this comment

felixlut commented Oct 15, 2023 • edited Loading

jdamata commented Oct 17, 2023

felixlut commented Oct 17, 2023

felixlut commented Oct 17, 2023 • edited Loading

jdamata commented Oct 17, 2023

felixlut commented Oct 18, 2023

felixlut commented Oct 19, 2023 • edited Loading

felixlut commented Oct 12, 2023 •

edited

Loading

felixlut commented Oct 15, 2023 •

edited

Loading

felixlut commented Oct 17, 2023 •

edited

Loading

felixlut commented Oct 19, 2023 •

edited

Loading