pronto icon indicating copy to clipboard operation
pronto copied to clipboard

Gitlab and consolidate_comments

Open tleish opened this issue 8 years ago • 15 comments

Does the consolidate_comments config option work for Gitlab? I have the following in .pronto.yml and it doesn't seem to be consolidating them.

all:
  exclude:
    - 'spec/**/*'
gitlab:
  slug: XXX
  api_private_token: XXX
  api_endpoint: XXX
  gitlab_api_httparty_options: "{verify: false}"
  format: "**%{runner} [%{level}]:** %{msg}"
max_warnings: 150
verbose: false
consolidate_comments: true

Or perhaps I'm mis-understanding the feature. The problem is, when pronto runs and finds multiple items to report, it posts multiple comments in Gitlab and ultimately several people get spammed with lots of emails instead of just one comment and one email. I was hoping this would solve it.

tleish avatar May 23 '17 23:05 tleish

@tleish this feature joins multiple comments for single line of code. For instance, given simple code with Rubocop offences

def foo_method
  a = 1
  b =1
end
$ pronto run -c origin/develop --staged -f text
app/foo.rb:2 W: Useless assignment to variable - `a`.
app/foo.rb:3 W: Useless assignment to variable - `b`.
app/foo.rb:3 W: Surrounding space missing for operator `=`.

As you can see there are 2 comments for line 3.

And if you run Gitlab formatter you will receive only 2 messages

$ pronto run -c origin/develop --staged -f gitlab
[#<struct Pronto::Comment sha="9e1c883f6762ad2a39c080b42e2cf03cff8c9c50", body="Useless assignment to variable - `a`.", path="app/foo.rb", position=2>, #<struct Pronto::Comment sha="9e1c883f6762ad2a39c080b42e2cf03cff8c9c50", body="- Useless assignment to variable - `b`.\n- Surrounding space missing for operator `=`.", path="app/foo.rb", position=3>]

Note that second message's body is joined comments. Without consolidate_comments: true in config file you would receive 3 separate comments.

ivanovaleksey avatar May 24 '17 06:05 ivanovaleksey

By the way consolidate looks like too official (and long) word IMO. Wouldn't it be better to use something more explicit - join - or something more git-like - squash? /cc @mmozuras

ivanovaleksey avatar May 24 '17 06:05 ivanovaleksey

@tleish could you expand on what kind of behavior you expect? A single comment under a pull request with all problems?

Even if consolidate_comments is not what you're looking for, we could consider introducing a separate feature/runner 🙂.

@doomspork would love your thoughts on @ivanovaleksey comment about consolidate vs. join/squash 🙂

mmozuras avatar May 27 '17 12:05 mmozuras

We use Gitlab. The complaint I'm getting my team is when they push a single commit, they get multiple emails (an email per issue reported by a pronto library, since they get an email each time a comment is left). If this do a single push, and then only get one email for that single push it would be better. Even, if they could get a single summarized email for single commit it would be better. Perhaps that single summarized comment to have hyperlinks which jump you to that line of code.

This might cause more confusion though if a summarized comment is left identifying 3 issues... then they fix 1 of the issues and push, but that original summarized comment identifying 3 issues still exists and would be visible during a merge request. So, I'm not sure this would work.

tleish avatar May 27 '17 19:05 tleish

I use Pronto with GitLab. But I don't use email as notification service. Instead I have Slack integration. In this case receiving multiple comments is totally fine IMO. Do you have an option to use your team messenger instead of email?

ivanovaleksey avatar May 27 '17 20:05 ivanovaleksey

@ivanovaleksey - we use Slack also, but have not integrated it with Gitlab. In your setup, does gitlab send the notifications to your personal slack, or to a shared slack channel?

tleish avatar May 27 '17 20:05 tleish

There are only two hard things in Computer Science: cache invalidation and naming things. -- Phil Karlton

@mmozuras I would personally be hesitant to rename a configuration option that's been released for the better part of a year "just because". If there's a influx of issues/bugs/complaints because it's confusing then I think it's warranted. Other than that it seems pedantic.

No one is typing consolidated_comments so often that it's taking away from productivity 😁

@tleish / @mmozuras maybe we could have a new "report" feature that gives you a single summary of all of your issues?

We had a similar issue with my previous employer with there being too many notifications but that to me is indicative of a bigger problem. When we had individuals producing dozens upons dozens of messages then there was a discussion around code quality, using git hooks, and running things locally.

doomspork avatar May 28 '17 15:05 doomspork

@tleish out of the box GitLab integration with Slack sends notifications to specified channel. But that is fine in my case since everyone could see code offences. I argree with @doomspork that things should be run locally too.

ivanovaleksey avatar May 28 '17 15:05 ivanovaleksey

I agree for something like Brakeman for strong security purposes. But for more opinionated static styling analysis, it can be considered more opinionated and "suggestions" rather than "failures". For failures, I'm okay users receiving multiple emails for each offense. However, for "suggestion" type analysis an alternative would be nice.

tleish avatar May 28 '17 18:05 tleish

@mmozuras I would personally be hesitant to rename a configuration option that's been released for the better part of a year "just because".

👍

@tleish / @mmozuras maybe we could have a new "report" feature that gives you a single summary of all of your issues?

Sure. I think a new formatter with this behavior would be fine 🙂. What do you think?

mmozuras avatar May 28 '17 19:05 mmozuras

@mmozuras if there's enough interest in a "report" I'd be happy to help nail down the features/requirements and take a stab at building it. @tleish would that work for you?

~~Going back over this I'm wondering if we couldn't solve this by whitelist/blacklist-ing certain "events"~~

doomspork avatar Jun 09 '17 17:06 doomspork

@doomspork that would be great 👍

Going back over this I'm wondering if we couldn't solve this by whitelist/blacklist-ing certain "events"

Could you expand on what you mean?

mmozuras avatar Jun 13 '17 19:06 mmozuras

@mmozuras disregard for now, it was a half baked thought 😀

doomspork avatar Jun 13 '17 20:06 doomspork

@doomspork - I'm interested in collaborating a solution.

tleish avatar Jun 13 '17 20:06 tleish

Sounds great @tleish! Do you want me to get a branch started that we can work with? How would you like to proceed? 😁

doomspork avatar Jun 13 '17 23:06 doomspork