Attest sonar wait flag#507
Conversation
| taskResponse, err := httpClient.Do(taskRequest) | ||
| if err != nil { | ||
| return "", err | ||
| wait := 10 // start wait period |
There was a problem hiding this comment.
we use https://github.com/hashicorp/go-retryablehttp to do the retries to Kosli api. See internal/requests.go.
It is possible to configure the retry logic on the retryable client like so
There was a problem hiding this comment.
Ah, I had looked at this but saw that it only retries on an error. I hadn't considered the ChatGPT workaround, which does seem doable (although would add even more complexity to what is already quite a complex command :P )
Also it would require starting two separate HTTP clients (there's no point having retries configured for any other request used in the command), I'm not sure how much overhead that would add?
There was a problem hiding this comment.
this made me realize that we actually use the kosli client to make requests to external 3rd parties (e.g getting image digest from registry, communicating with bitbucket) although not used for all 3rd party interactions.
The kosli client is built on top of retryablehttp and gives standard ways of logging and error handling as well as other small consistency things like adding the user agent header.
It might be useful to standarize how we make requests to 3rd party tools. That said, the client currently does not have a way to specify a custom retry policy.
| cmd.Flags().StringVar(&o.projectKey, "sonar-project-key", "", sonarProjectKeyFlag) | ||
| cmd.Flags().StringVar(&o.serverURL, "sonar-server-url", "https://sonarcloud.io", sonarServerURLFlag) | ||
| cmd.Flags().StringVar(&o.revision, "sonar-revision", o.commitSHA, sonarRevisionFlag) | ||
| cmd.Flags().IntVar(&o.maxRetries, "max-retries", 0, sonarMaxRetriesFlag) |
There was a problem hiding this comment.
would it be better to default the max-retries to something like 3?
if the objective is to get data eventually if it is not available yet, then it is probably more user friendly to retry by default rather than fail first and then make the user specify a number of retries.
On another note, in this context, it might be better to have a wait flag instead of retry. You wait (by retrying) until the results are available or a certain timeout is reached. The user will never know how many retries should work, but might have a reasonable guess on how long they want to wait (timeout).
There was a problem hiding this comment.
Ah, yeah, that would work. My first go checked the flag value before starting the retry loop, so I needed the default to be 0 for that - but with the current implementation it should be fine with a non-zero default.
I did originally have the flag as a wait time, but Jon suggested using retries instead - I think his reasoning was that we may have other commands in the future that would benefit from using retries (e.g. that use other async APIs), and it would be good to have a common flag. But I do see your point - particularly for this command which might need quite long wait periods if SonarQube is having significant delays.
(Although if using a wait period, I guess the retryablehttp above wouldn't be usable? But that's not really an issue)
There was a problem hiding this comment.
we have maxAPIRetries flag for kosli api requests. I suppose for other async 3rd part interactions, the need would be similar to sonar (keep waiting for results to become available or timeout).
There was a problem hiding this comment.
Based on comments from Sami, I decided to go back to using a --max-wait flag instead of --max-retries.
Reasoning: Users won't know how many retries or how long they might need to wait for Sonar to make the scan results available - but they will have an idea of the maximum length of time they're willing to stall their pipeline for. It's much easier to let them give us that time, rather than making them guess how many retries it might take.
The flag defaults to a 30s wait time (but obviously if the scan results are available sooner this doesn't apply). Not sure if this is correct - maybe a shorter default would be better?
2 participants
Based on comments from Sami, I decided to use a
--max-waitflag instead of--max-retries.Reasoning: Users won't know how many retries or how long they might need to wait for Sonar to make the scan results available - but they will have an idea of the maximum length of time they're willing to stall their pipeline for. It's much easier to let them give us that time, rather than making them guess how many retries it might take.
The flag defaults to 30s, which should cover "standard" delays on SonarQube's end (although not cases where they're experiencing significant delays). If the scan is available sooner than that, the command doesn't wait.