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

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

Open smokedlinq opened this issue 2 years ago • 6 comments
trafficstars

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 avatar Nov 09 '23 02:11 smokedlinq

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.

smokedlinq avatar Nov 09 '23 02:11 smokedlinq

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

jdamata avatar Dec 18 '23 06:12 jdamata

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

tbutler-qontigo avatar Feb 19 '24 14:02 tbutler-qontigo

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.

smokedlinq avatar Feb 22 '24 03:02 smokedlinq

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

freeranger avatar Feb 25 '24 08:02 freeranger

@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).

smokedlinq avatar Feb 26 '24 14:02 smokedlinq