jdamata / terraform-provider-sonarqube

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

Fix `sonarqube_permission_template` resource when `default` is `true` #220

Open smokedlinq opened 8 months ago

smokedlinq commented 8 months ago

When default is enabled on a sonarqube_permission_template resource it is producing the wrong URL to the API. This is because the modified URL is being used on the call to the resourceSonarqubePermissionTemplateSetDefault method.

Example error:

Error: error setting Sonarqube permission template to default: API returned an error: Unknown url : /api/permissions/create_template/api/permissions/set_default_template
│ 
│   with module.sonarqube_configuration.module.permissions.sonarqube_permission_template.default,
│   on modules/sonarqube/configuration/permissions/main.tf line 26, in resource "sonarqube_permission_template" "default":
│   26: resource "sonarqube_permission_template" "default" {
smokedlinq commented 8 months ago

I'm not 100% sure this is the correct way to fix this in golang, I'm still very new but it's my best attempt. If a more appropriate solution is available, please let me know.

jdamata commented 6 months ago

Looks ok to me. Can you add a test case for this? Also apologies for the late review

tbutler-qontigo commented 4 months ago

@smokedlinq If you add a test case for this then it can be merged. Thanks

smokedlinq commented 4 months ago

Shouldn't the TestAccSonarqubePermissionTemplateDefaultTemplate test case be handling this if it could, but according to the comment it can't. I am guessing this error isn't manifesting because it happens during the apply and not the plan.

freeranger commented 4 months ago

Shouldn't the TestAccSonarqubePermissionTemplateDefaultTemplate test case be handling this if it could, but according to the comment it can't. I am guessing this error isn't manifesting because it happens during the apply and not the plan.

Personally I am not a big fan one one test to do everything - a test should have only one (logical) reason to fail. The more you have the code testing, the more reasons there are for failure and the harder it will be to determine why a test is suddenly failing. I am not sure what this test is actually testing, but it is passing today so doesn’t cover your scenario. Can you add another test that would? A unit test mocking certain aspects may be more appropriate than an integration test

smokedlinq commented 4 months ago

@freeranger That's what I went to do initially, but there are no unit tests. In the one unit test for testing the default flag, there is a comment:

// Must be set to plan as its not possible to destroy a template that is the current default.
// This results in the error: It is not possible to delete the default permission template for projects

I take this to mean I can't do one that requires apply. It may be possible but I'm not sure how, and I am very new to golang so I'm completely unfamiliar with creating a unit test for the HTTP client. Ideally, I'd like to get this fixed so we can remove our workaround (using a terraform_data resource with a custom script calling the API).