AmadeusITGroup / sonar-stash

Stash (BitBucket) plugin, a pull-request decorator which allows to integrate SonarQube violations directly into your pull-request
MIT License
165 stars 82 forks source link

Plugin does not handle 401 codes from Stash #175

Closed Burrch3s closed 6 years ago

Burrch3s commented 6 years ago

After having the plugin successfully work for a few repos and successfully commenting on PRs, I accidentally set the webhook up for one that the user did not have read access to throwing a 401 back from stash. I believe that even though the first message or attempt from this plugin received a 401, it still tried to send the rest of the requests, since the user this runs as became locked out.

It's not a huge issue, I just logged in, unlocked the user and gave them read access to the repo, but it would be nice if this ever happened again by mistake, that the plugin would stop at the first 401 and not get the user locked out

t-8ch commented 6 years ago

@Burrch3s As you are talking about webhooks: are you sure you are using this plugin and not some other sonar stash integration?

But I also agree that this plugin should abort in case of errors.

Burrch3s commented 6 years ago

@t-8ch Sorry for the late reply, but yes I am using the plugin. I understand where you're coming from, I suppose in trying to explain in full the situation I may have put in some non-relevant information. The webhook triggers the Jenkins builds, but that's not relevant to the problem. The problem being that I accidentally ran analysis on a repo it did not have access to read yet, so when this plugin tried to comment on the Pull Request, 401 errors were thrown and I believe it did not handle these errors with the first 401 and continued to try and comment on the PR until the Jenkins user was locked out for repeated requests to the PR it didn't have access to. Sorry for any confusion.

t-8ch commented 6 years ago

@Burrch3s ok, this clears it up. I am not sure yet whether we should abort the complete build or only the sonar-stash analysis. Probably only the analysis.

t-8ch commented 6 years ago

Hi @Burrch3s , the 401 does get handled in the current development version.