jdamata / terraform-provider-sonarqube

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

[Quality Gates] Operators for conditions that have grade rating thresholds only works with GT #171

Closed lpcruz closed 11 months ago

lpcruz commented 1 year ago

Hi there,

tldr

SonarQube will use "is worse than" operator for conditions that have grade rating thresholds. It's not exactly clear which op to use when provisioning this condition. It turns out that GT is the only way I found it worked and provisioned what I wanted.

For context, I was able to successfully provision my quality gate; however, I wanted to surface this to see if this was the intended behavior.

Terraform Version

Run terraform -v to show the version. If you are not running the latest version of Terraform, please upgrade because your issue may have already been fixed.

terraform -v
Terraform v1.3.1
on darwin_arm64

Affected Resource(s)

Please list the resources as a list, for example:

Terraform Configuration Files

// Example main.tf that will not successfully apply.

resource "sonarqube_qualitygate" "main" {
    name = "My Quality Gate"
    is_default = false

    condition {
        metric    = "new_coverage"
        op        = "LT"
        threshold = "50"
    }

    condition {
        metric    = "new_duplicated_lines_density"
        op        = "GT"
        threshold = "3"
    }

    condition {
        metric    = "new_maintainability_rating"
        op        = "LT"
        threshold = "1"
    }

    condition {
        metric    = "new_reliability_rating"
        op        = "LT"
        threshold = "1"
    }

    condition {
        metric    = "new_security_hotspots_reviewed"
        op        = "LT"
        threshold = "100"
    }

    condition {
        metric    = "new_security_review_rating"
        op        = "LT"
        threshold = "1"
    }
}

resource "sonarqube_qualitygate_project_association" "main" {
  gatename   = sonarqube_qualitygate.main.name
  projectkey = var.projectKey
}

Expected Behavior

I'd assume if I use op = "LT" that that would be synonymous and/or evaluate to SonarQube's "is worse than" operator.

Actual Behavior

This will actually not work and yield:

│ Error: resourceSonarqubeQualityGateCreate: Failed to synchronise quality gate conditions: addOrUpdateCondition: Failed to create condition 'new_reliability_rating': API returned an error: Operator LT is not allowed for this metric.

Steps to Reproduce

Please list the steps required to reproduce the issue, for example:

  1. terraform apply

References

Are there any other GitHub issues (open or closed) or Pull Requests that should be linked here? For example:

jdamata commented 1 year ago

Yah thats super weird. Checking API docs, there is only two options for the condition operators: image

Maybe the API is lagging behind?

How did your workaround work via GT? Wouldn't that condition not be what you wanted?

tbutler-qontigo commented 1 year ago

Hi both

I think this may be down to poor documentation/poor API and not a problem with the provider as such.

If you try to create conditions via the UI then it looks like the numeric ones only allow "less than" and the alpha ones only allow "is worse than". There don't seem to be cases where you can choose either less or greater than for any given metric. When you think about it, it doesn't make sense to either - why would you want to fail if maintainability is BETTER than B or if code coverage is GREATER than 80%?

When you retrieve them via the API, they are all surfaced as numbers with LT or GT as appropriate so I guess they are just using one mechanism to represent both types of metric. So a new code reliability rating of is worse than A in the UI is returned from the API as new_reliability_rating GT 1 when returned from the API, where A=1, B=2 etc. I can't see any API call you can make to determine which ones support which operators so I guess they must have some kind of mapping in the UI to ensure they present the right option per metric type - and of course present a dropdown of A-D for the alpha ones.

I guess the provider could maintain a similar internal mapping and validate early if the wrong operator is used with a given metric - but that is prone to getting out of sync with whatever hard coded list Sonar must be using in their UI. You may be able to "cheat" for some of them if you retrieve the built in Sonar way gate and you can determine based on the operator what is/is not allowed. Doesn't help for their non-default ones though :/ Or perhaps it could trap these errors (which unfortunately would only manifest at apply time and not at plan time) and return a more friendly message. Or do nothing - there's only so much we can do to make up for a poor API :)

For sure Sonar should update their documentation to make it clear which conditions support which operator and ideally provide an API end point where we can retrieve this information.

lpcruz commented 1 year ago

@tbutler-qontigo @jdamata Thanks both!

@tbutler-qontigo Thanks for all that context; it makes sense and yeah I pretty assumed that this was more of an issue at the SonarQube API level.

I completely agree with ensuring the provider doesn't have to accommodate for that since that would be a maintenance burden for sure.

That said, for now - I'm going to leave my main config to "as-is" and explain to the team / link to this specific issue ticket as a reference. I'm specifically going to clarify around this:

So a new code reliability rating of is worse than A in the UI is returned from the API as new_reliability_rating GT 1 when returned from the API, where A=1, B=2 etc.

I am curious, is there any way for the provider to just make op optional for grade-rated conditions? It seems that at a SonarQube level "is worse than" is the one operator so maybe it'd make sense for the provider to acknowledge that when using a grade-rated condition.

Does that seem like a first pass at clarifying this at a provider level?

Happy to hear both your thoughts, cheers.

tbutler-qontigo commented 1 year ago

hmmm.. Perhaps the conditions could be refactored on the assumption that you only have LT for A/B/C/D metrics and only have GT for the others. On that basis, you would probably want something more like:

   "reliability_rating" = {
      threshold = "B"
    },
    "new_coverage" = {
        threshold = "50"
    }

and give up the op completely in all cases. The provider would have to determine it is an A/B/C/D metric and convert it to the appropriate number and add in the GT operator, otherwise just add the LT operator as it would be assumed to be a numeric metric And of course coming back from the API it would need to translate them the other way...

We are making an assumption though - which does hold at the moment but may not in the future. I suppose another option would be to keep op but make it optional so that if there was an alpha that allowed LT or a numeric that allowed GT then the caller could provider the right operator...though coming back from the API we would be unable to map it correctly as we wouldn't know without hard coding a map.

And of course this would be a breaking change to the provider :)

lpcruz commented 1 year ago

Totally hear ya. Thanks @tbutler-qontigo

Maybe one quick pass is to make a note of this in the provider docs? Maybe like a known "gotchas" or something.

FWIW, I'd be happy to draft something up if y'all are taking contributions. Just a suggestion!

jdamata commented 1 year ago

Thanks for the explanation @tbutler-qontigo !!

@lpcruz Yep a docs contribution would be awesome. Probably belongs on this page?

https://registry.terraform.io/providers/jdamata/sonarqube/latest/docs/resources/sonarqube_qualitygate

https://github.com/jdamata/terraform-provider-sonarqube/blob/master/docs/resources/sonarqube_qualitygate.md