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

Ensure commit sha is always passed to Message #24

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

tobischo
Copy link

In #18 the issue is raised the gitlab API cannot be correctly addressed with the gem in the state it is right now.

This seems to be happening because the commit sha is set to nil in any case

Message.new(path, line, level, offence['message'], nil, self.class)

Lines would provide the commit sha correctly for the change and the error caused by it, but those are only accessed for non fatal errors.

The commit sha is also stored at the top level @patches variable which can provide it for the commit that was last pushed/on which the check is running. In for fatal errors it is possible then to use this commit message to attach the messages

@tobischo tobischo requested a review from a team as a code owner May 10, 2021 15:27
@ashkulz
Copy link
Member

ashkulz commented May 11, 2021

I'm not sure about this change, as I looked at pronto-rubocop and pronto-haml and it has the same logic -- so if it's a bug, it's more wide-spread than this linter. Can you reproduce it with other linters?

@tobischo
Copy link
Author

tobischo commented May 11, 2021

You are right, for pronto-rubocop it is not there and it currently is also not causing issues for us.

I know that I added it for pronto-golang

But let's go through the code:

The runners are executed here:
https://github.com/prontolabs/pronto/blob/999d0b92fc2f916cb1c77fa4f792c31428a0b560/lib/pronto.rb#L66
https://github.com/prontolabs/pronto/blob/999d0b92fc2f916cb1c77fa4f792c31428a0b560/lib/pronto/runners.rb#L20

We are using the -f gitlab option, so
https://github.com/prontolabs/pronto/blob/999d0b92fc2f916cb1c77fa4f792c31428a0b560/lib/pronto/formatter/formatter.rb#L19

And the gitlab formatter
https://github.com/prontolabs/pronto/blob/999d0b92fc2f916cb1c77fa4f792c31428a0b560/lib/pronto/formatter/gitlab_formatter.rb#L3
is coming from the commit formatter
https://github.com/prontolabs/pronto/blob/999d0b92fc2f916cb1c77fa4f792c31428a0b560/lib/pronto/formatter/commit_formatter.rb#L10
which is coming from
https://github.com/prontolabs/pronto/blob/999d0b92fc2f916cb1c77fa4f792c31428a0b560/lib/pronto/formatter/git_formatter.rb#L3

And the messages are mapped to comments here:
https://github.com/prontolabs/pronto/blob/999d0b92fc2f916cb1c77fa4f792c31428a0b560/lib/pronto/formatter/git_formatter.rb#L74

and the commit_sha is coming from the message when mapping it:
https://github.com/prontolabs/pronto/blob/999d0b92fc2f916cb1c77fa4f792c31428a0b560/lib/pronto/formatter/git_formatter.rb#L65

And the commit_sha in the message, if left to nil, needs to come from the line:
https://github.com/prontolabs/pronto/blob/999d0b92fc2f916cb1c77fa4f792c31428a0b560/lib/pronto/message.rb#L18

For the fatal errors it therefore needs to get the commit_sha but it does not need it for other types of errors, so we could change it to not pass in the commit_sha for the places where we have a line object.
That's probably also why we do not need it for pronto-rubocop and pronto-haml.

@ashkulz
Copy link
Member

ashkulz commented May 12, 2021

Thanks for the detailed analysis, @tobischo! I'll try to take a look at it over the weekend 👍

@tobischo
Copy link
Author

Hey @ashkulz
did you maybe have some time in between to take a look at this change?

@istisiki
Copy link

Can confirm this is still a problem.
I've added pronto-eslint to our Github Actions recently and it's gives the same error.

/opt/hostedtoolcache/Ruby/2.7.3/x64/lib/ruby/gems/2.7.0/gems/octokit-4.21.0/lib/octokit/response/raise_error.rb:14:in `on_complete': POST https://api.github.com/repos/ORG_NAME/REPO_NAME/commits//comments: 404 - Not Found // See: https://docs.github.com/rest (Octokit::NotFound)
	from /opt/hostedtoolcache/Ruby/2.7.3/x64/lib/ruby/gems/2.7.0/gems/faraday-1.8.0/lib/faraday/middleware.rb:19:in `block in call'
	from /opt/hostedtoolcache/Ruby/2.7.3/x64/lib/ruby/gems/2.7.0/gems/faraday-1.8.0/lib/faraday/response.rb:59:in `on_complete'
	from /opt/hostedtoolcache/Ruby/2.7.3/x64/lib/ruby/gems/2.7.0/gems/faraday-1.8.0/lib/faraday/middleware.rb:18:in `call'
	from /opt/hostedtoolcache/Ruby/2.7.3/x64/lib/ruby/gems/2.7.0/gems/octokit-4.21.0/lib/octokit/middleware/follow_redirects.rb:73:in 
	...

The runners we are using are:

  • pronto-rubocop
  • pronto-eslint

Removing pronto-eslint resolves the issue for me but I would really want to have that in our CI.

@leonardoalemax
Copy link

this will solve my problem.

@ashkulz
Copy link
Member

ashkulz commented Jan 4, 2022

Sorry, this got lost in my to do list. I'll take a look at it in the evening.

@tobischo
Copy link
Author

@ashkulz Gentle reminder for this 🙂

@ashkulz
Copy link
Member

ashkulz commented Jun 29, 2022

@tobischo thanks for the reminder, I'll review this on the weekend. Sorry for the delay 🙈

@tobischo
Copy link
Author

Maybe this weekend? 😄

@tobischo
Copy link
Author

tobischo commented Aug 6, 2022

Or this one? 😉

@ashkulz
Copy link
Member

ashkulz commented Aug 13, 2022

Sorry, there's been a lot of sickness in sequence in the family. I'll do it this weekend, I promise 🙈

@tobischo
Copy link
Author

No need to explain or apologize.
As far as I am concerned I am basically asking you to spend your spare time for free on something for me.

Therefore, no rush, just gentle reminders as I also know how easily something like this can be forgotten about

@tobischo tobischo force-pushed the fix/return-commit-sha-in-message-correctly branch from c7d9be8 to 2f39674 Compare June 2, 2023 10:26
@tobischo
Copy link
Author

tobischo commented Jun 2, 2023

Alright, I resolved the conflict that got in from another change in the meantime.

Gentle reminder for a review as well

@tobischo
Copy link
Author

It's been a while 😄
Any chance you might find time in the near future?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants