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

AMI stack: ensure pull requests / disable direct pushing #11

Closed
ghost opened this issue Jun 1, 2017 · 6 comments
Closed

AMI stack: ensure pull requests / disable direct pushing #11

ghost opened this issue Jun 1, 2017 · 6 comments
Labels

Comments

@ghost
Copy link

ghost commented Jun 1, 2017

@petermr (and others) historically pushed directly to repositories hosted on GitHub under the Contentmine organization (e.g. https://github.com/ContentMine/svg/ ). This works fine for solo developers, but doesn't scale so well to multi-person teams. It bypasses the code review steps that are afforded by using pull requests ("PRs"), and misses out on much of the value added by code hosting services such as GitHub, GitLab, BitBucket, etc.

Now that 3-4 developers will be working intensively on the AMI stack, @petermr and I agreed last week to disable direct pushing for the AMI stack's ContentMine repositories, in favour of PRs. This will facilitate publicly-visible code review and CI, and reduce merge conflicts.

Instead, contributors to any AMI stack repo should follow The GitHub Flow, such that each developer has:

  • their own remote on GitHub, to which they push, and from which they can issue PRs against the corresponding ContentMine repo;
  • a local repository, with remote-tracking branches for both of those remotes, and possibly also for colleagues' remotes.

GitHub flow diagram

(Source.)

Ideally, PRs should be performed by a contributor other than the one responsible for the PR (e.g. I would merge @petermr's PRs; he would merge mine), to ensure scrutiny; but due to the small size of the team, developers should remain able to merge their own PRs in case time pressure is high and other team members are unavailable.

This is sometimes known as a "forking workflow". It is basically the same as the "Integration-Manager Workflow" described https://git-scm.com/book/en/v2/Distributed-Git-Distributed-Workflows, except with PRs in place of the "Integration Manager".

I will put this in place as @petermr and I agreed last week, unless anyone objects.

Pinging @ContentMine/administrators for feedback. If you approve, please just "+1" or "thumbs up" this post. If you disapprove, please give constructive criticism :) Thanks!

@ghost ghost changed the title Ensure code review happens Ensure code review happens in AMI stack Jun 2, 2017
@ghost ghost changed the title Ensure code review happens in AMI stack AMI stack: ensure pull requests / disable direct pushing Jun 2, 2017
@tarrow
Copy link

tarrow commented Jun 2, 2017

👍 This is a nice idea

@ghost
Copy link
Author

ghost commented Jun 2, 2017

Thanks :) I'd like to go ahead with this on Monday, if no-one objects, so that I can get on with Tilburg.

@ghost ghost self-assigned this Jun 2, 2017
@ghost ghost added the question label Jun 5, 2017
@ghost
Copy link
Author

ghost commented Jun 6, 2017

Monday has been and gone... Thumbs up so far (thanks!) from:

  • blahah
  • chreman
  • jkbcm
  • tarrow

OK with you, @petermr ?

@ghost
Copy link
Author

ghost commented Jun 7, 2017

@petermr confirmed today in person. Will put this in place ASAP.

@ghost ghost closed this as completed Jun 7, 2017
@ghost
Copy link
Author

ghost commented Jun 8, 2017

@jkbcm and @tarrow: as of this evening, after a few hours' work with me to handle some existing remotes with divergent histories, @petermr now has remotes under his namespace on GitHub for all the AMI stack modules, and will be pushing to those. Until he gets into the swing of creating feature/bugfix branches, etc, which will hopefully be in the coming weeks, his pushes may be to the master branches on those repositories.

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

No branches or pull requests

1 participant