RFD 175 SmartOS integration process changes discussion about rfd HOT 27 OPEN

any necessary "glue" for merging them into illumos-joyent (such as manifest changes) can
be dealt with by a PR against illumos-joyent at the time of the daily merge.

I don't think we need to do anything special for manifest changes. As part of our normal merge process we're used to updating the illumos-joyent manifest to reflect the changes in the upstream pkg manifests. So, as long as the illumos-gate manifests are properly updated (which is normal when going in to illumos-gate), then we should be fine on the merge.

from rfd.

from rfd.

Just wanted to note that I read this and it all seems reasonable. The networking changes can rely pretty heavily on hardware for testing, and I don't have enough machines to run SmartOS and OmniOS/OI. Though the compromise of developing the change on illumos-joyent and pushing to gate would probably work well. Also, expanding on simnet and the nettest suite will also help a lot in terms of testing everything above the driver. The real hurdle right now is I need to take some time to get our network changes upstream -- as some of our delta is built on changes that still aren't upstream. Some of that is "easy", like hardware VLAN filtering, and some of it will require more work, such as zone-aware link handling (IPD3).

Only noticed a small typo in the final graph: "and [t]hat seems".

Thanks for doing this.

from rfd.

Thanks Ryan. The intent is not for this to get in the way of important work. So, if it's a choice between a significant amount of legwork to upstream stuff to just get some important changes in, feel free to target illumos-joyent instead.

(It would of course be great to see more of the networking stuff upstreamed though.)

from rfd.

any necessary "glue" for merging them into illumos-joyent (such as manifest changes) can be dealt with by a PR against illumos-joyent at the time of the daily merge.

I'm now sure how the sequencing of this would work, because presumably there'd be cases where the daily merge would fail due to an issue with the sync that the original author of the illumos-gate change knew about. The alternative, of just pushing the merge and leaving the gate in a broken state until the PR was integrated seems unfortunate.

Instead, I wonder whether we ought to have a ticket filed in advance of the illumos-joyent integration to explain the extra work that will be needed for the merge, assuming it's more than just a manifest sync.

The author of the change then would have to to contact the folks doing the merge in advance of integrating the change to illumos joyent, to be sure they're prepared, and that ticket would need to be added to the merge commit message. Where this suggestion falls down, is how such a change gets code-reviewed: we don't currently code-review any changes being made during a merge (because hopefully they're all simple) Perhaps that will continue to be the case?

from rfd.

We could file a ticket indeed (I thought I mentioned that but maybe not). But a PR is even more useful, as it shows the exact set of changes and is usable for code review.

Either way there should be a useful way to give a heads up to whoever's on daily merge duty.

I think for more complex changes we'd scale up to do a proper code review - maybe ahead of time too.

from rfd.

Ultimately the back merge is one of the trickiest parts here, we might have to play it by ear to see how bad things are there.

from rfd.

But a PR is even more useful, as it shows the exact set of changes and is usable for code review.

A PR used only for code review purposes then? I don't think we'd be able to push such a PR, we'd instead want it rolled directly into the merge commit.

from rfd.

Why not? We currently regularly have merge commits that break the build, so I don't think there's an argument around git bisect and the like.

from rfd.

Ok, I didn't think that was by design, but sure 👍

from rfd.

I agree it's probably not by design but it is common. Alternatively, could we provide some tooling to help? Could we push daily merge to a branch, and allow incoming merge fixes to rebase on to that and run through a quick build? Seems like there's a bunch of stuff we can do to help, but not yet clear how much effort is worth it.

from rfd.

Just providing some info ... answering Qs that were in the ~os channel, in case it helps:

• to have a guard on PRs being merged without an "IA" label, one would need to implement a "check" (a server that GH would call when PRs are updated) that would return "fail" if the PR had no IA label. Then the repo can be configured to require that this check is in a passing state before allowing merge.
  Or, perhaps for starters, you could use the honour system.

• To get the commit message without the blank line, you'll have to have and use a tool that uses the API to do the merge. I.e. using the merge button will break the commit message format rule, as specified, because GH will always put that blank line in.

from rfd.

Maybe we should think about having that blank line anyway then.

from rfd.

I personally would be happy to have the blank line. For multi-bug commits, having the first line be the main bug/synopsis in the commit seems sufficient (rather than, say, trying to cram everything onto the first line with a list of space-separated Jira commits)

TOOLS-2361 is in process which will require an "integration-approval" label to be applied to any PR before the PR can integrate.

The CLI "squash and merge" tool that I'm working on looks for this PR label, and auto-generate a commit message that includes "Reviewed by:" and "Approved by:" tags. Users can of course still choose to use the UI to squash+merge if they wish.

I think our periodic upstream gate merges pose a different problem. Specifically, most Joyent repositories have branch guards that check the following:

• at least one reviewer (not the submitter) must review PRs. This is a branch-protection rule, which allows us to declare a list of 'administrator' users that can bypass it, so I think we'd need to add our merge users to that list. That seems preferable to another suggestion I had, which was to implement our own code-review-counting status check which would always pass for those users.

• merges can only be "squash + merge", not traditional merges - will that impact the daily merge process? (unlike the above, there doesn't seem to be a way around that)

from rfd.

Squash and merge is going to be a big problem yes. We definitely need traditional merges. Can we merge via the CLI or is that setting over-ridden too?

We'll have to allow traditional merges if not. Yay github!

from rfd.

I'm not sure if I am following this completely, but this sounds similar to how we have gerrit setup today for the daily merge. I have permission to push directly to gerrit with no CR/IA approval. I do have to be trusted not to abuse this capability. Presumably once we switch to github, I could just continue to push the daily merge to the master branch?

from rfd.

I think it's more of a worry about the UI being presented: given that we have to allow traditional merges for our daily-merge process, normal users might accidentally do a traditional merge rather than gerrit's default, which was to squash+merge. I'll see whether the CLI can override a squash+merge restriction (in a test repository) but would be surprised if github allowed that.

The alternative would be to allow pushes without PRs (as we do today) - perhaps that gets around the restrictions here.

from rfd.

Right - the squash+merge button restrictions only apply to PRs, so we're ok there.

The branch protection rules would need to have the 'Include administrators> Enforce all configured restrictions above for administrators. ' check box un-ticked, and for Jerry & I (and any other merge folks) to be added as administrators for the repository.

Here's what that looks like, first trying to push to master with normal branch protection rules enabled:

timf@iorangi-eth0 (master) git push [email protected]:timfoster/hook-test.git
Enumerating objects: 15, done.
Counting objects: 100% (15/15), done.
Delta compression using up to 8 threads
Compressing objects: 100% (9/9), done.
Writing objects: 100% (12/12), 1.01 KiB | 115.00 KiB/s, done.
Total 12 (delta 5), reused 0 (delta 0)
remote: Resolving deltas: 100% (5/5), completed with 1 local object.
remote: error: GH006: Protected branch update failed for refs/heads/master.
remote: error: At least 1 approving review is required by reviewers with write access.
To github.com:timfoster/hook-test.git
 ! [remote rejected] master -> master (protected branch hook declined)
error: failed to push some refs to '[email protected]:timfoster/hook-test.git'

Now allowing administrators to bypass the checks:

timf@iorangi-eth0 (master) git push [email protected]:timfoster/hook-test.git
Enumerating objects: 15, done.
Counting objects: 100% (15/15), done.
Delta compression using up to 8 threads
Compressing objects: 100% (9/9), done.
Writing objects: 100% (12/12), 1.01 KiB | 115.00 KiB/s, done.
Total 12 (delta 5), reused 0 (delta 0)
remote: Resolving deltas: 100% (5/5), completed with 1 local object.
To github.com:timfoster/hook-test.git
   40a3fe3..01b17ec  master -> master
timf@iorangi-eth0 (master)

from rfd.

Specifically, in both of the above cases, my test repository only had the "Allow squash merging" and "Allow rebase merging" options selected, I did not have "Allow merge commits" selected. The change that I pushed directly to master was a merge commit:

timf@iorangi-eth0 (master) git log -1
commit 01b17ecad5455181c7e2bea455614452272ae7b1 (HEAD -> master)
Merge: 1262430 c260d05
Author: Tim Foster <[email protected]>
Date:   Wed Nov 6 16:07:52 2019 +0000

    merge upstream conflict

from rfd.

from rfd.

However, trying the merge process via PR will fall foul of the 'squash and merge' restriction, even when using the API:

timf@iorangi-eth0 (another-pr)  echo '{"commit_title": "This is a commit", "commit_message": "message body", "sha":"8ee7b8678665f3aff36e7cf507fd53f4a4ef7e01" , "merge_method": "merge"}' |  hub api -X PUT --input - /repos/timfoster/hook-test/pulls/24/merge
{"message":"Merge commits are not allowed on this repository.","documentation_url":"https://developer.github.com/v3/pulls/#merge-a-pull-request-merge-button"}timf@iorangi-eth0 (another-pr)

from rfd.

There is this: Restrict who can push to matching branches

Ah (my 'timfoster/hook-test' repo not being an Enterprise repo didn't have that option) I worry that the "Require pull request reviews before merging" restriction would mean that users on that guest-list would still be required to use PRs, and that would prevent us from being able to push merges.

from rfd.

tl;dr: Listing illumos-gate mergers under "Restrict who can push to matching
branches" does not do what we want here.

I played with the "master" branch protection on joyent/play. First set it
to our common "master" branch protection settings:

$ jr github-settings set-branch-protection play
Updated "play" repo "master" branch protection (https://github.com/joyent/play/settings/branches)

Which means that a direct push of a commit to master will fail:

$ git ci -am "tweak Makefile"
[master f01a83e] tweak Makefile
 1 file changed, 1 insertion(+), 1 deletion(-)

$ git push origin master
Counting objects: 3, done.
Delta compression using up to 8 threads.
Compressing objects: 100% (3/3), done.
Writing objects: 100% (3/3), 297 bytes | 297.00 KiB/s, done.
Total 3 (delta 2), reused 0 (delta 0)
remote: Resolving deltas: 100% (2/2), completed with 2 local objects.
remote: error: GH006: Protected branch update failed for refs/heads/master.
remote: error: At least 1 approving review is required by reviewers with write access.
To github.com:joyent/play.git
 ! [remote rejected] master -> master (protected branch hook declined)
error: failed to push some refs to '[email protected]:joyent/play.git'

I update the branch protection to enable "Restrict who can push to matching branches" and added my account:

I had originally had the hope (as MikeG did) that this was a mechanism to
exclude the listed accounts/teams from the "thou must only push to master via a
PR". Alas, listing "trentm" here doesn't allow me to just push to master:

$ git push origin master
Counting objects: 3, done.
Delta compression using up to 8 threads.
Compressing objects: 100% (3/3), done.
Writing objects: 100% (3/3), 297 bytes | 297.00 KiB/s, done.
Total 3 (delta 2), reused 0 (delta 0)
remote: Resolving deltas: 100% (2/2), completed with 2 local objects.
remote: error: GH006: Protected branch update failed for refs/heads/master.
remote: error: At least 1 approving review is required by reviewers with write access.
To github.com:joyent/play.git
 ! [remote rejected] master -> master (protected branch hook declined)
error: failed to push some refs to '[email protected]:joyent/play.git'

from rfd.

Assuming we still want:

1. "master" branch protection at all (I believe we do to guard against force push to "master")
2. to require a PR and a review on that PR to push to master for regular commits (this, together with [x] Include administrators, guards against accidental administrators pushing directly to master by mistake)

The only ways I can think of doing this are with a process like this:

1. Call the GH API to disable the "require a PR and a review" setting on the branch protection.
2. Do the "git push origin master" to push the prepared illumos-gate merge.
3. Call the GH API to re-enable the branch protection settings.

Here is a proof of concept. I already had jr github-settings set-branch-protection REPO to help set our standard branch protections on all our repos. I added a --jerry mode flag to have it set "Jerry Mode" :) for doing the merge:

First I cannot push:

$ git push origin master
Counting objects: 3, done.
Delta compression using up to 8 threads.
Compressing objects: 100% (3/3), done.
Writing objects: 100% (3/3), 298 bytes | 298.00 KiB/s, done.
Total 3 (delta 2), reused 0 (delta 0)
remote: Resolving deltas: 100% (2/2), completed with 2 local objects.
remote: error: GH006: Protected branch update failed for refs/heads/master.
remote: error: At least 1 approving review is required by reviewers with write access.
To github.com:joyent/play.git
 ! [remote rejected] master -> master (protected branch hook declined)
error: failed to push some refs to '[email protected]:joyent/play.git'

Then I use Jerry mode, push, then restore settings:

$ jr github-settings set-branch-protection play --jerry
Updated "play" repo "master" branch protection [jerry mode: no PR required for push] (https://github.com/joyent/play/settings/branches)

$ git push origin master
Counting objects: 3, done.
Delta compression using up to 8 threads.
Compressing objects: 100% (3/3), done.
Writing objects: 100% (3/3), 298 bytes | 298.00 KiB/s, done.
Total 3 (delta 2), reused 0 (delta 0)
remote: Resolving deltas: 100% (2/2), completed with 2 local objects.
To github.com:joyent/play.git
   b8563dc..bb85aaa  master -> master

$ jr github-settings set-branch-protection play
Updated "play" repo "master" branch protection (https://github.com/joyent/play/settings/branches)

We could wrap up that jr github-settings set-branch-protection play --jerry in something more convenient. Obviously it does leave the potential to accidentally forget to update the settings.

from rfd.

An slight tweak on the process above would be to temporarily remove the [x] include administrators setting instead of the required PR and a review setting. I think that would probably work better: the GH API setting is clearer (a bool instead of an object); the side-effect of forgetting to restore settings only impacts people who are admins.

from rfd.

I will only countenance this if it's actually commited as a "--jerry" flag.
This sounds like a great compromise. Can we have a cron job to check the flag is set back after the daily jerry?

from rfd.

Can we have a cron job to check the flag is set back after the daily jerry?

We could have a daily Jenkins job called "jerry".

from rfd.