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

Possibly add commit linting for core commits #54

Closed
evanlucas opened this issue Jul 20, 2016 · 11 comments
Closed

Possibly add commit linting for core commits #54

evanlucas opened this issue Jul 20, 2016 · 11 comments

Comments

@evanlucas
Copy link

I have been working on using core-validate-commit to lint a commit based on a github patch. Maybe this is something we could integrate into the bot?

/cc @jbergstroem

@phillipj
Copy link
Member

Any thoughts on how this should look like in a PR? A couple of options comes to mind:

  1. Use the GH status API to indicate success/errors like we're used to with e.g. Travis CI
  2. Make the bot create a comment if linting errors/warnings exists

The benefit of option 2 is that we can actually print all the messages your core-validate-commit module generates, that's not as easy with option 1 as we can only display one line of text IIRC.

..or maybe a combination of both would be ideal?

@jbergstroem
Copy link
Member

2 can be spammy. The status api can provide a message as well -- if users push updates to pr and expect the bot to give it new info about linting it would probably annoy a lot of people that get notifications on issue updates.

@jbergstroem
Copy link
Member

Also, since we can link away to $url, we can notify users of the issues over there.

@williamkapke
Copy link
Contributor

williamkapke commented Jul 25, 2016

@jbergstroem, one way to avoid spamming people is to have the bot update an existing comment. My approach on other things was to have the bot append to the author's issue message.

@jbergstroem
Copy link
Member

jbergstroem commented Jul 25, 2016

@williamkapke ok -- not sure if that sends another email or not. How would the author know its updated?

Edit: Ok, so, editing the top message. Just so i better understand all options: what are the downsides of using the build status api?

@evanlucas
Copy link
Author

I do think it would be nice to be able to update whenever a push is made. I am not sure the best path to go though. I was planning to do some proof of concepts with both and see which one we like better?

@jbergstroem
Copy link
Member

@evanlucas sounds good to me.

@phillipj
Copy link
Member

@jbergstroem said:
Just so i better understand all options: what are the downsides of using the build status api?

We're not able to display much of the errors and warnings the core-validate-commit module generates, as GH truncates status descriptions. As an example I just tried to set the following description on a PR:

Line #20: Fixes must be a url, not an issue number.
Line #3: Line should be <= 72 columns.

which ended up looking like this:

image

To overcome that we could provide an URL with the status which will make the "Details" link appear, but that means we'll have to create an endpoint somewhere which renders the complete list of linting issues for a commit - IMO it smells too complex for a first shot at commit msg linting.

@evanlucas said:
I do think it would be nice to be able to update whenever a push is made.

Have a look at scripts/display-travis-status.js#L10 where we listen for PR updates.

And creating/updating a status can be done with the githubClient as shown in lib/push-jenkins-update.js#L22. In the screenshot shown above, these options was sent:

{
  user: 'phillipj',
  repo: 'webhooks-testing',
  sha: 'dff8fcf880017cd212cdc5c28931420dd1988201',
  context: 'testing-statuses',
  state: 'failure',
  description: 'Line #20: Fixes must be a url, not an issue number.\nLine #3: Line should be <= 72 columns.'
}

@phillipj
Copy link
Member

phillipj commented May 6, 2017

Should we still do this? @jbergstroem has expressed some concern about commits not being ready for linting until just before landing, therefore displaying an inline PR status before then isn't that valuable?

@gibfahn
Copy link
Member

gibfahn commented May 6, 2017

I think this could still be useful, as long as we don't require people to have this green before someone lands it.

Related: nodejs/node#12107 (cc/ @vsemozhetbyt)

@maclover7
Copy link
Contributor

Moving this to nodejs/build#793

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

No branches or pull requests

6 participants