RFD 175 SmartOS integration process changes discussion #143
Comments
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. |
|
On Wed, Oct 30, 2019 at 06:33:45AM -0700, Jerry Jelinek wrote:
> 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.
Right, that's a bad example (I only recently found out that we do this).
It'd be more for things where e.g. we changed some kernel function and
need to fix up code only we carry.
thanks
john
|
|
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. |
|
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.) |
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? |
|
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. |
|
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. |
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. |
|
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. |
|
Ok, I didn't think that was by design, but sure 👍 |
|
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. |
|
Just providing some info ... answering Qs that were in the ~os channel, in case it helps:
|
|
Maybe we should think about having that blank line anyway then. |
|
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:
|
|
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! |
|
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? |
|
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. |
|
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: Now allowing administrators to bypass the checks: |
|
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: |
|
I know that I've accidentally pushed to master in a repo because of the
"allow administrators" setting. I'd prefer that we avoid that option, but
if it is the only reasonable one to use I won't stand in the way.
There is this:
Restrict who can push to matching branchesSpecify people, teams or apps
allowed to push to matching branches. Required status checks will still
prevent these people, teams and apps from merging if the checks fail.
Could we just add Jerry or a pseudo-user that Jerry uses for his updates to
the list that is allowed to push to master? But maybe that requires a PR
plus membership in that list.
Mike
…On Wed, Nov 6, 2019 at 10:12 AM Tim Foster ***@***.***> wrote:
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:
***@***.*** (master) git push ***@***.***: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 ***@***.***:timfoster/hook-test.git'
Now allowing administrators to bypass the checks:
***@***.*** (master) git push ***@***.***: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
***@***.*** (master)
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#143>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ACETNBJNLHZW2FVTNYM3FSDQSLUHRANCNFSM4JGMONKQ>
.
|
|
However, trying the merge process via PR will fall foul of the 'squash and merge' restriction, even when using the API: |
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. |
|
tl;dr: Listing illumos-gate mergers under "Restrict who can push to matching I played with the "master" branch protection on joyent/play. First set it Which means that a direct push of a commit to master will fail: 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 |
|
Assuming we still want:
The only ways I can think of doing this are with a process like this:
Here is a proof of concept. I already had First I cannot push: Then I use Jerry mode, push, then restore settings: We could wrap up that |
|
An slight tweak on the process above would be to temporarily remove the |
|
I will only countenance this if it's actually commited as a "--jerry" flag. |
We could have a daily Jenkins job called "jerry". |
https://github.com/joyent/rfd/tree/master/rfd/0175
The text was updated successfully, but these errors were encountered: