Test-infra: Help Requested: git guru for sig-docs v1.15 release.

[jimangel]

Hey all 👋

We're having some issues merging master into our dev branch. @zacharysarah, @zparnold, and I can't figure out why @makoscafee's PR to resolve conflicts (https://github.com/kubernetes/website/pull/14950) doesn't actually resolve conflicts (https://github.com/kubernetes/website/pull/14176).

I outlined what I thought was the process: https://gist.github.com/jimangel/288a1a442ba52566ff825bfd828d0a00

Can someone please help @kubernetes/sig-docs-en-owners understand what is needed to resolve the dev-1.15 conflicts?

We're also not sure how to resolve the following:

• invalid commit messages
  
  
  
  • https://github.com/kubernetes/website/pull/14176#issuecomment-502856551
  
  
  • https://github.com/kubernetes/website/pull/14940#issuecomment-502605818
  
  

Thanks!

[gochist]

Is there someone who has a clone of makoscafee:resolve-conflicts-dev1.15 or dev-1.15 before squashed?

[stevekuznetsov]

GitHub seems to think that this commit merged that PR to resolve conflicts, but when I clone the repo I can't see it:

$ git log -n 1
commit 7e194986c8adad16aab14a75298208a4ca6aa6b4 (HEAD -> master, origin/master, origin/HEAD)
Author: Jim Angel <[email protected]>
Date:   Mon Jun 17 14:23:51 2019 -0500

    adding Barnie's write permissions to k/website (#14948)

    A tad late in the release cycle for this, should have been done sooner to streamline the docs release team.

$ git show d38df79767f698487935444a24c3f2499fe493a7
fatal: bad object d38df79767f698487935444a24c3f2499fe493a7

[stevekuznetsov]

Has the branch dev-1.15 been force-pushed since then?

[stevekuznetsov]

It is possible that one of these force-pushes overwrote that history:

[stevekuznetsov]

While the GitHub UI allows us to look at the commit that got orphaned from the graph, it does not allow it to be fetched:

$ git fetch origin d38df79767f698487935444a24c3f2499fe493a7
error: Server does not allow request for unadvertised object d38df79767f698487935444a24c3f2499fe493a7

If someone has a local copy of the work (the PR author?) we could resolve this.

[stevekuznetsov]

I was also able to pull down the commit object from the GitHub API, so we could apply that manually, too. Even GZipped it's 26MB so I can't upload here, going to try to add it to the SIG Testing drive.

[stevekuznetsov]

The diff is smaller than the patch -- you should be able to git apply this

commit.diff.gz

[jimangel]

@stevekuznetsov thanks for looking into this!

I think you're spot on about the force push (/cc @zacharysarah).

Do we need @makoscafee's local copy to resolve this?

How could we have avoided getting into this situation in the first place?

[stevekuznetsov]

I don't think you need the local copy, no. You just need a version of the repository with 9cf4dcaa62bfb529a30227e52b9479a0f7ec1b00 still around. That's the version of the dev-1.15 branch that existed before @makoscafee landed his PR. What I suggest is:

git checkout dev-1.15
git reset --hard 9cf4dcaa62bfb529a30227e52b9479a0f7ec1b00
git apply commit.diff # after downloading the attachment from my comment and unzipping it
git push --force dev-1.15

That will reset the dev-1.15 branch to the version it was in when that pull request from @makoscafee landed.

How could we have avoided getting into this situation in the first place?

If you require that developers create branches in their local forks and then use pull requests to update the dev-1.15 branch, instead of allowing force pushes, that would have not led to this. If it's still required that developers are able to _push_ to the branch, that is distinct from allowing history overwrites, and could also be permitted and still not lead to this issue.

[stevekuznetsov]

In general when using Prow, if jobs are turned on for the branch and branch protection is used, branches that have presubmit jobs will not allow for direct pushes without passing tests and will not allow history overwrites, period.

[stevekuznetsov]

Oops, didn't mean to close this!

[makoscafee]

The funny thing is if I merge that branch to my own fork branch dev-1.15 which is the exact copy of upstream dev1.15 conflicts are resolved. 🤷‍♂

[jimangel]

What I suggest is:

git checkout dev-1.15
git reset --hard 9cf4dcaa62bfb529a30227e52b9479a0f7ec1b00
git apply commit.diff # after downloading the attachment from my comment and unzipping it
git push --force dev-1.15

That will reset the dev-1.15 branch to the version it was in when that pull request from @makoscafee landed.

Would this be done based off of k/website clone or a local fork?

Also, if this gets us back to square one, I don't understand how this will allow us to resolve conflicts. Before the force push that Zach did, we saw similar behavior (Barnie's PR from a local fork merged in does not resolve conflicts).

Does the conflict resolution have to be done out of k/website? Or, another way to ask, can a fork not resolve a remote / upstream conflict?

If you require that developers create branches in their local forks and then use pull requests to update the dev-1.15 branch, instead of allowing force pushes, that would have not led to this.

I thought this is how we were operating, with the exception of a couple of @zacharysarah force pushes while he was looking into the problem.

[stevekuznetsov]

Would this be done based off of k/website clone or a local fork?

k/website as it is right now does not contain pullable refs for the commits that got overwritten so you'd need a local fork with that work still around.

Before the force push that Zach did, we saw similar behavior. Or, another way to ask, can a fork not resolve a remote / upstream conflict?

I'd have to see the commits in question to comment. In theory you should be able to propose a commit that does not cause merge conflicts but the devil is in the details.

[gochist]

I guess the conflict was not resolved because @makoscafee's PR(kubernetes/website#14950) was merged to the dev-1.15 in the 'squash' merge mode which is the current Tide configuration for k/website.

[gochist]

I found the location of configuration. Adding a label tide/merge-method-merge on the PR resolving conflicts will work.

[nikhita]

:wave: looks like this was solved already and can be closed? :)

[jimangel]

Sorry @nikhita, I've been meaning to summarize our resolution and close. I'll close now and fill in a comment when time permits today.

[nikhita]

@jimangel No problem at all! Just came across this while looking through the list of open issues...

[jimangel]

For this issue there were 2 solutions:

• Short term (1.15)
  
  
  
  • Give the release lead elevated privileges to write directly to branch
  
  
  • (by default) Authors already allowed edits on PRs, a requirement to resolve conflicts remotely
  
  
  • PR was created directly against k/website vs local fork
  
  
• Long term (1.16+)
  
  
  
  • @gochist's suggestion of adding the label tide/merge-method-merge should also work
  
  
  • this method allows for the PR to resolve conflicts vs. squashing as a "new" PR
  
  
  • resolution would occur on the local fork and be PR'd in
  
  
  • @simplytunde will test this method first for 1.16, and we can always leverage the short term fix if needed.
  
  

@gochist provided this excellent visual to explain why the tide/merge-method-merge label should work: