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

Limit push access to LTS staging branches to backport team #199

Closed
MylesBorins opened this issue Apr 14, 2017 · 23 comments
Closed

Limit push access to LTS staging branches to backport team #199

MylesBorins opened this issue Apr 14, 2017 · 23 comments

Comments

@MylesBorins
Copy link
Contributor

This came up in the first backport team meeting. One way that we can be sure of the health of the staging branch is to make sure only backport-team is pushing to it.

Any objections?

@gibfahn
Copy link
Member

gibfahn commented Apr 15, 2017

In my experience people pushing to the wrong branches is nearly always a mistake, anything that makes it harder for me to accidentally push to the wrong branch SGTM.

@refack
Copy link

refack commented Apr 22, 2017

I call for a more radical solution: clone into a separate repo.
This will allow fine grained control of access. And in term of workflow it's as simple as changing a word in the git command line.
Main question is where issues and PR are managed:

  1. stay in node with the managed by appropriate labels
  2. move here with better focus and more accurate distribution list

@bnoordhuis
Copy link
Member

I call for a more radical solution: clone into a separate repo.

Intensely annoying for code archeology, something I do a lot.

@gibfahn
Copy link
Member

gibfahn commented Apr 23, 2017

I call for a more radical solution: clone into a separate repo.
This will allow fine grained control of access. And in term of workflow it's as simple as changing a word in the git command line.

That seems like it'll make everything much more complicated for no obvious benefit.

If we limit access to the branch then if you push to it by mistake it'll just be rejected, and you'll realise your mistake. That seems like the ideal way to do it.

Having a separate repo loses us many of the benefits of Git, it means you need two clones of Node (doubles your disk usage), and that you can no longer just git cherry-pick HASH to backport.

This will allow fine grained control of access.

Protecting the branches should allow equally fine-grained access control?

And in term of workflow it's as simple as changing a word in the git command line.

You mean for the clone? Yeah sure, it's only slightly more complicated for that, but that's something you do once. In terms of backporting it will make it much more complex.

EDIT: In fact what would probably happen is everyone would just have nodejs/lts and nodejs/node as two remotes of the same repo, which leads to what is in essence the same as having protected branches.

@refack
Copy link

refack commented Apr 23, 2017

Think about it again.
Clone the repo, not start from commit 0.

@bnoordhuis

Intensely annoying for code archeology, something I do a lot.

I agree that archaeology is very useful, I use is allot as well to try and figure out what led to a certain solution.
But I don't think we'll lose that, it'll be the same repo just under a different name. Only thing we'd "lose" is issue number parity, but since we put full URLs in commit messages, we won't really lose that.

@gibfahn

Having a separate repo loses us many of the benefits of Git, it means you need two clones of Node (doubles your disk usage), and that you can no longer just git cherry-pick HASH to backport.

No, you probably already use "two" remote repos, your fork, and the upstream one? it's just a matter of where you target your push/pull

EDIT: In fact what would probably happen is everyone would just have nodejs/lts and nodejs/node as two remotes of the same repo, which leads to what is in essence the same as having protected branches.

Exactly! except not "everyone" will have these two, only people who do backporting. And it will allow better management for issues/PRs and a better focused notification audience.

@refack
Copy link

refack commented Apr 23, 2017

Another benefit I tought of:

  1. If we remove the backporting branches from the main repo git will be more responsive locally (we don't want to get to the chromium situation, where git status take ~2 minutes)

What I've seen in places that did this sort of split was a conceptual change in the way the devs thought. When it's unnecessary vNext was freed from backward compatibility. While vLTS got dedicated dev resources to solve version specific issues in the right context.

An example nodejs/node#12580: whatever the eventual solution will be, IMHO this is a v7 issue and should have been solved with v7 as the focus, and not as a v7 follows master solution.

@refack
Copy link

refack commented Apr 23, 2017

This ties in with my thoughts that we should invest in vLTS more #203 (comment)

@gibfahn
Copy link
Member

gibfahn commented Apr 23, 2017

If we remove the backporting branches from the main repo git will be more responsive locally (we don't want to get to the chromium situation, where git status take ~2 minutes)

Is it the number of branches that makes git take a long time? I thought it was the lstat on all the files. It's certainly pretty quick right now: git status 0.04s user 0.09s system 168% cpu 0.074 total.

Exactly! except not "everyone" will have these two, only people who do backporting. And it will allow better management for issues/PRs and a better focused notification audience.

Everyone who commits into master should (hopefully) backport it to the relevant release lines if there is a merge conflict, so realistically most contributors will need both (so you won't get much git status benefit). You want the original committer to fix any backport merge conflicts because they should be the person who best understands the change.

What I've seen in places that did this sort of split was a conceptual change in the way the devs thought. When it's unnecessary vNext was freed from backward compatibility. While vLTS got dedicated dev resources to solve version specific issues in the right context.

The whole backporting/release process is designed to minimise differences between versions, and to minimise mental load about which commits are where. Where possible we change things in master, and where possible we backport to all active release lines. When it's time for a new release we can just branch from master (or vNext). What I'd like to do is have less difference between different versions of Node (i.e. backport more stuff), and I don't see how this would help that.

An example nodejs/node#12580: whatever the eventual solution will be, IMHO this is a v7 issue and should have been solved with v7 as the focus, and not as a v7 follows master solution.

I think nodejs/node#12580 is something that's relevant to Node 8 as much as Node 7. --debug had to be dropped without deprecation cycles because of v8, but --inspect --debug-brk existed before --inspect-brk, and there's no reason to just remove that at the same time.

@refack
Copy link

refack commented Apr 23, 2017

Is it the number of branches that makes git take a long time? I thought it was the lstat on all the files. It's certainly pretty quick right now: git status 0.04s user 0.09s system 168% cpu 0.074 total.

For git status probably, but for git diff, git commit, and git log the refs are opened (not to mention git blame)... But I agree that marginal.

Everyone who commits into master should (hopefully) backport it to the relevant release lines if there is a merge conflict, so realistically most contributors will need both (so you won't get much git status benefit). You want the original committer to fix any backport merge conflicts because they should be the person who best understands the change.

The point is that most of the things should not be backported. Main use-case for master is new development.

The whole backporting/release process is designed to minimise differences between versions, and to minimise mental load about which commits are where. Where possible we change things in master, and where possible we backport to all active release lines. When it's time for a new release we can just branch from master (or vNext). What I'd like to do is have less difference between different versions of Node (i.e. backport more stuff), and I don't see how this would help that.

I think this is the crux of the matter; I'd like to see more difference between versions 😉. More innovation in vNext / more stability in vLTS, and you are right my suggestion will help that.
Otherwise why have semver-major version changes at all?

@gibfahn
Copy link
Member

gibfahn commented Apr 23, 2017

The point is that most of the things should not be backported. Main use-case for master is new development.

Most Node.js development is minor and patch stuff, if it doesn't go to master, where should it go?

I think this is the crux of the matter; I'd like to see more difference between versions

You'd like to see more major changes, or less minor/patch backporting? The former is limited by the mass of existing Node.js code, the developers of which are (understandably) unwilling to have to change every year, and not by the backporting process. The latter is facilitated by the current model, and I'm not really sure why you wouldn't want things to be backported.

The point of semver is to allow backporting without affecting stability, and it's worked pretty well so far. A Node.js developer wants existing APIs not to break, and new APIs to work across every version of Node his app/library is likely to have to run on. What we currently do is tailored around that.

@gibfahn
Copy link
Member

gibfahn commented Apr 23, 2017

Anyway, splitting into multiple repos is orthogonal to this, so probably worth discussing in a separate issue.

@MylesBorins
Copy link
Contributor Author

I'm a huge -1 on a new repo. Distributed nature of git aside it is project history, and it belongs in the repo.

I believe that outstanding questions tangential to the private branches can be discussed in other issues. I think we should decide as a WG that we want to vote on this in tomorrows meeting. Assuming we vote to move forward we should bring to ctc Wednesday.

@refack
Copy link

refack commented Apr 24, 2017

As an outsider I think I'm +1 on limited access branches

@jasnell
Copy link
Member

jasnell commented Apr 24, 2017

I'm +1 to limiting access to the staging and release LTS branches to the LTS WG. (that said, if we're goiing to pursue this, we should formalize the LTS WG... and I'm thinking it makes sense to formally attach it to the Release team in some way...)

@mhdawson
Copy link
Member

I'm +1 to limiting as well.

@MylesBorins
Copy link
Contributor Author

MylesBorins commented Apr 24, 2017

@nodejs/lts has agreed to move forward with this.

@nodejs/ctc can you please chime in here if you have any objections

@cjihrig
Copy link
Contributor

cjihrig commented Apr 25, 2017

Sounds like a good idea to me.

@shigeki
Copy link

shigeki commented Apr 25, 2017

What to do upgrading openssl-1.0.2 for security fix against lts staging branches in the future?
It would not be a kind of the current backport procedures after we upgrade openssl-1.1.x.

@sam-github
Copy link
Contributor

@shigeki We would open a PR targeting a lts-staging branch if there was a security fix (or any kind of fix) that applied to an LTS release but did not apply to master.

@sam-github
Copy link
Contributor

@MylesBorins when sec fixes like nodejs/node@cae9eb35f0 are made, they seem to get pushed directly to LTS release branches (not even going through the lts-staging branch). @gibfahn pointed out to me in side-channel that may be @shigeki's concern.

I don't know the process there. I think perhaps both the backport team and the release team may need push access? Or perhaps the CTC should get push access?

Then again, 4 members of the LTS WG/backporting team (at least) are on the security team so can land security patches on LTS branches easily (James, Myles, Michael, and myself, as is Anna). And all CTC members have the ability to override permissions in any github.com/nodejs/ repo, so I don't think protecting the branches will prevent anything that needs doing, or even slow it down.

@MylesBorins
Copy link
Contributor Author

MylesBorins commented Apr 25, 2017

@sam-github your final point summates my thoughts. I say lets move forward with the plan the way it is and augment if we find things are slowing down or blocked

@shigeki
Copy link

shigeki commented Apr 26, 2017

Thanks for clarification. I agree with trying to start a PR based process in such cases.

@MylesBorins
Copy link
Contributor Author

no objections from CTC

backporting team created
v4.x-staging + v6.x-staging are protected

FWIW Organization and repository administrators can still push

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

9 participants