Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Summary comment must be resolved by a hand before pull request could be merged. #106

Closed
exxbrain opened this issue Feb 26, 2020 · 11 comments
Labels
enhancement New feature or request

Comments

@exxbrain
Copy link

exxbrain commented Feb 26, 2020

Summary comment must be resolved by a hand before pull request could be merged. So now pull request decoration at least in gitlab negatively affects a merge process when pull request couldn't be merged automatically after successful completion of a pipeline.

Steps to reproduce the behavior:

  1. Navigate to your gitlab project’s settings page, select the Only allow merge requests to be merged if all threads are resolved check box and hit Save for the changes to take effect.
  2. Make merge request without issues and click a button "Merge when pipeline succeeds"

Expected behavior
Pull request should be automatically merged.

Screenshots
Screenshot 2020-02-26 at 14 06 47

Screenshot 2020-02-26 at 14 13 34

Screenshot 2020-02-26 at 13 55 20

Software Versions

  • Gitlab
  • SonarQube Version: 7.9.2 (build 30863)
  • Plugin Version: 1.2.1-RC3

Suggestion
I think that it will be good to resolve summary comment thread automatically if there are no issues found.

@exxbrain exxbrain added the bug Something isn't working label Feb 26, 2020
@exxbrain exxbrain changed the title The gitlab comment must be resolved by a hand before it could be merged. Summary comment must be resolved by a hand before it could be merged. Feb 26, 2020
@exxbrain exxbrain changed the title Summary comment must be resolved by a hand before it could be merged. Summary comment must be resolved by a hand before pull request could be merged. Feb 26, 2020
@mc1arke
Copy link
Owner

mc1arke commented Feb 26, 2020

This sounds like a feature request rather than a bug: you've enabled decoration, but your current pipeline then prevents decorated MRs from being auto-merged. I presume you're asking for the summary comment to posted in another way?

@kortov
Copy link

kortov commented Feb 26, 2020

I guess it's similar to #97

@exxbrain
Copy link
Author

I presume you're asking for the summary comment to posted in another way?

If it is possible then yes. But current solution is nice as well except the case when there are no issues.

@exxbrain
Copy link
Author

exxbrain commented Feb 26, 2020

I guess it's similar to #97

@kortov I think you are right. But I posted this bug because current functionality is conflicting with gitlab settings. The solution may be the same: just don't post a comment with zero issues, or as @mc1arke said - in another way.

@exxbrain
Copy link
Author

exxbrain commented Feb 26, 2020

Gitlab provides 2 endpoints - discussions and notes:

  1. https://docs.gitlab.com/ee/api/discussions.html
  2. https://docs.gitlab.com/ee/api/notes.html.

Now we use discussions. I think we can try notes. Probably it will be enough to fix both issues. Also I see some as I think useful flags: resolvable, resolved, noteable_id.

@kortov
Copy link

kortov commented Feb 26, 2020

Let's sum up all of this. I guess it's all should work like checks on github - if check is failed - it prevents merge, and if passed then no matter what the check may write in comments (e.g codecov) it doesn't prevent merge.
So I guess the plugin should work like this. If quality gate passed - write something not preventing merge (note, resolved thread, autoresolved by plugin thread etc)
I guess this is not implemented now but there are options in gitlab api to work it out and we only should pick the suitable one. Am I right?

@exxbrain
Copy link
Author

exxbrain commented Feb 26, 2020

Yes. We need to solve 2 requirements by picking options from gitlab api for the case when no issues found:

  1. No notification.
  2. Note (discussion) should be resolved. (This will allow auto-merge).

By the way

If quality gate passed

This doesn't mean that there are no issues. I think code reviewer should decide in this case, i.e. summary comment better not to be resolved from my point of view.

Also I think discussion is not an option because I don't know what I should discuss under summary comment.

@mfolnovic
Copy link

From my point of view, it makes sense to use Discussion API for issues and Notes API for summary. Which is probably what you thought, right?

@exxbrain
Copy link
Author

exxbrain commented Mar 14, 2020

The fix for the problem is in the #126.

mareksimunek pushed a commit to mareksimunek/sonarqube-community-branch-plugin that referenced this issue Mar 5, 2021
@MCMicS
Copy link

MCMicS commented Jun 25, 2021

Change #226 will auto resolve the discussion if quality gates passes

@mc1arke mc1arke closed this as completed Jun 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
6 participants