Document PR Acceptance Process #3185

Open
scott-w opened this Issue Sep 16, 2016 · 16 comments

Projects

None yet

5 participants

@scott-w
Member
scott-w commented Sep 16, 2016 edited

As we've released v3, we've had some time to reflect and have discussed ideas for what the process for PRs and code releases should be.

The current state of play is:

Submitting PRs

  • Bug fixes and new features should have PRs based off next
  • Documentation updates for the current version should have PRs based off master
  • Documentation updates for new features e.g. v3.1 should have PRs based off next

This would require some minor updates to CONTRIBUTING.md to account for this.

Approving PRs

Originally, we used 2 👍 before changes were accepted. There was a temporary policy of 1 👍 for documentation changes, simply due to the fact that it was either myself of @rafde that were submitting the PRs and availability of the core team was limited.

With Github's new Review Process, it looks like we'll move to incorporate it directly into our workflow and use 2 "Approvals" as a way of handling this. I think that, now we've gotten over the major rush of changes for v3, we can also go back to requiring 2 approvals of documentation changes too.

Release Process

This part I'm a bit unclear on so it'll be worth putting something down that we can refer to in discussion.

@denar90
Member
denar90 commented Sep 16, 2016

@scott-w love it. Also we should make https://github.com/marionettejs/backbone.marionette/projects active. It will help us to understand who is working on which task.

@rafde
Member
rafde commented Sep 17, 2016

@denar90 I think active task can work through Assignment
I'm still a little fuzzy on how to use projects.
Maybe the first project can be "The road to deprecating CompositeView"

@scott-w for sure. Establish the checks and balances.

@scott-w scott-w added a commit to scott-w/backbone.marionette that referenced this issue Sep 17, 2016
@scott-w scott-w Initial contributor proposal #3185
I've re-written some of the contributor file to try and cover some of the changes to our process.
8b1a795
@scott-w scott-w added a commit to scott-w/backbone.marionette that referenced this issue Sep 17, 2016
@scott-w scott-w Initial contributor proposal #3185
I've re-written some of the contributor file to try and cover some of the changes to our process.
b910a00
@scott-w
Member
scott-w commented Sep 17, 2016

Question: what happens if you give an approval and commits are submitted subsequently? I'm assuming they no longer count and have to be re-approved?

@JSteunou
Contributor

About PR I would rather consider fixes for the current version of any kind (doc or src code) on master and new things of any kind on next

@denar90
Member
denar90 commented Sep 17, 2016

What about merge conflicts? If I made fix in master and same file was changed in next...

@paulfalgout
Member

Merge conflicts are going to be a reality I think. Otherwise we get into the situation where we have an urgent bug that needs fixing but we can't release because the next branch isn't in a good state. Things will really start to get hairy when we want to start work on v4. We may need to start the 3rd branch at that point. But ideally we can just keep the breaking changes short and sweet and get through it quickly.

@scott-w
Member
scott-w commented Sep 17, 2016

So something like:

  • master - for any patch releases and docs against the stable release
  • next - for any new feature development
  • ??? - for breaking changes moving to v4

For releasing a patch in this case, we can do:

  1. Branch off master into 3.0.1
  2. Release 3.0.1

For releasing a major build:

  1. If necessary, release a final patch from master as above
  2. Merge or overwrite next onto master (prefer merging)
  3. Branch master into 3.1.0
  4. Release 3.1.0

For releasing a breaking build:

  1. If necessary, release a final major build from next as above
  2. Merge or overwrite ??? onto master (prefer merging)
  3. Branch master into next and 4.0.0
  4. Release 4.0.0

It's a bit extensive and I've not covered the case of us maintaining multiple versions. Whether we feel we have the capacity to maintain older major/minor versions is also something we need to consider and state explicitly.

@JSteunou
Contributor
JSteunou commented Sep 17, 2016 edited

Something like that. We are using git flow at work, help a lot for cases like this one. One rule is that master is almighty and never directly changes. A support branch is used to collect all fixes. When it is time to release a bug fix version support is merged into master. For minor things there is a branch develop (or next). When it is time to release a minor version develop is merged into master. And for each release master is merged back onto the other branches so they are always up to date. For a major incoming release you can create another branch from develop, the same principle apply. You can go with an unlimited number of branches as long as you merge down on master for a release then merge back up on the other.

@scott-w
Member
scott-w commented Sep 19, 2016

I see, at our place we use something much simpler as we have a much smaller team (4 is the most I've had at a given point in time) with all PRs going straight into master and a separate release branch for pushing code into production.

We don't differentiate between bug fixes and new features except in really rare cases as we find we're able to push out fixes to new features fast enough that it's not a major issue. In addition, we find that the complexity of managing multiple branches outweighs the benefits.

@paulfalgout
Member

I think it makes sense to merge relevant docs directly into master because it's the only way we're going to be able to push them outside of rolling a release which is time consuming.

Previously we had breaking master minor and major where both patch and new non-breaking features were added to minor.. but major got left so far behind that it was very difficult to keep up.. So then at some point we build next off of minor and cherry-picked what we could from major. Then we added a patch branch for bug fixes.

I kind of think we just keep next for everything new and possibly leave breaking changes unmerged??.. until we are ready to make a commitment to a short timeline for a breaking changes release at which point we make a patch branch and make next v4.

@JSteunou
Contributor

So it will goes:

  • fixes of all kind (src & doc) master
  • new things of all kind into next
  • breaking changes as pending PR until ready for next

should be very fine

@paulfalgout paulfalgout added a commit to paulfalgout/backbone.marionette that referenced this issue Oct 12, 2016
@scott-w @paulfalgout scott-w + paulfalgout Initial contributor proposal #3185
I've re-written some of the contributor file to try and cover some of the changes to our process.
9846d29
@scott-w
Member
scott-w commented Oct 23, 2016

I'm thinking if we merge #3241 then we should update CONTRIBUTING.md with the instructions from that.

@scott-w scott-w added a commit to scott-w/backbone.marionette that referenced this issue Oct 23, 2016
@scott-w scott-w Wrote up a basic release process #3185
I've sketched up what we could use for the release process just using two branches. I'm a bit stumped into how we'd do v4 using this, however so I've left that out.
f057e3d
@scott-w scott-w added a commit to scott-w/backbone.marionette that referenced this issue Oct 23, 2016
@scott-w scott-w Added a few tweaks to the contributor readme #3185 5d67889
@paulfalgout
Member

Could we just add esfix to a precommit hook or something?

@denar90
Member
denar90 commented Oct 24, 2016

@paulfalgout I like the idea. Can we use smth like https://github.com/observing/pre-commit ?

@paulfalgout
Member

I'm not sure where this goes, but we need to add something about the stuff that gets merged into master should get merged back into next as well

@scott-w
Member
scott-w commented Nov 7, 2016

We can put it into CONTRIBUTING.md to put it out there then figure it out later. I'm assuming we just do a git merge or do you prefer the rebasing strategy?

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