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

CLI wrapper to rebase, pre-fill and run CI on PRs #325

Closed
silverwind opened this issue Feb 9, 2016 · 4 comments
Closed

CLI wrapper to rebase, pre-fill and run CI on PRs #325

silverwind opened this issue Feb 9, 2016 · 4 comments

Comments

@silverwind
Copy link

Spawned off #324 (comment)

The basic idea would be a CLI script that does:

  1. Take a PR number argument
  2. Pull patches from Github
  3. Aks y/N if commits shall be squashed
  4. Rebase commits and apply metadata (use GitHub API to parse for LGTMs)
  5. Preview commit messages and ask y/N if they shall be further edited in $EDITOR
  6. Start a Jenkins job, possibly authorized through a user's SSH key or other token
  7. Sleep the process until Jenkins is complete
  8. If CI is green, ask y/N whether the commits shall be landed

The only step on which I'm unsure on how to do would be step 5. Can anyone think of a git workflow to add metadata to existing commits without rebase --interactive?

cc: @jbergstroem

@Trott
Copy link
Member

Trott commented Feb 10, 2016

Maybe add a commit message linting/validation step that makes sure the final message:

  • has a first line that is not empty and is not longer than 50 chars
  • has a first line that matches a regular expression along the lines of this (untested): /^[a-z,]+: \w.*/
  • has a blank second line
  • has a non-blank third line
  • has a blank line before the metadata
  • has metadata consistent with what it knows about the PR (e.g., PR-URL exists and is correct)
  • has at least one Reviewed-by metadata line
  • individuals in Reviewed-by metadata are in fact collaborators (basically compare against a whitelist of people who can give a LGTM that counts)
  • all non-metadata lines are shorter than 72 chars

@jbergstroem
Copy link
Member

Excellent list by @Trott. I'd like to add:

  • metadata ordering (suggesting: PR-URL -> Fixes -> Refs -> Reviewed-By)

It'd be really nice to have a LGTM validation/"check" (ok/not ok to land); but I think it's too hard to make realistic LGTM counts seeing how new commits could introduce new code.

Trott added a commit to Trott/io.js that referenced this issue Apr 26, 2016
Convention has coalesced around putting the PR-URL as the first metadata
item in pull requests, so much so that collaborators are now flagging
other collaborators' commit messages when they don't do that. So let's
make it official and document that the PR-URL should be first.

Refs: nodejs/build#325
Refs: nodejs#6385
@refack
Copy link
Contributor

refack commented May 21, 2017

Cross ref: #705

There's an issue of racing with humans/concurrent jobs. That's where the concept of a queue comes into play. (#324 (comment))

@Trott
Copy link
Member

Trott commented Mar 22, 2019

Closing due to long period of inactivity. Feel free to re-open if this is a thing. I'm just trying to close stuff that has been ignored for sufficiently long that it seems likely it's not something we're going to get to.

@Trott Trott closed this as completed Mar 22, 2019
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

No branches or pull requests

4 participants