search

betterscientificsoftware / bssw.io

Better Scientific Software Homepage
https://bssw.io
Other
140 stars 90 forks source link

Address issues with updating of the preview branch #744

Closed bernhold closed 3 years ago

bernhold commented 3 years ago

In an email conversation, @bartlettroscoe notes that we really should 1) protect the preview branch to limit the people who can commit directly to it. In theory, there should be no direct commits to preview, but in practice, there may be cases where manual intervention is required to deal with conflicts, so we don't want to completely ban it. 2) we should also prevent PRs from being squashed or rebased when they're merged to master, as this is more likely to lead to merge conflicts with prior versions of PRs on preview.

@bartlettroscoe to provide details of how to implement.

Tasks:

bartlettroscoe commented 3 years ago

To protect the 'preview' branch, we need to define another team (i.e. 'bssw-io-preview-admin') and then use that team to allow write access to 'preview' at:

To disallow squashing and rebasing on PR merge, you can uncheck the boxes "Allow squash merging" and "Allow rebase merging" under the section "Merge button" at:

There is no good reason that I can think of to allow rebasing when merging to 'master'. However, if people are sloppy about their commits, it is good to squash when merging the PR. Therefore, we should experiment to see if Git and GitHub will allow this. From a file patch point of view, Git should allow this but we will have to see.

Given the above, right away we should do:

@betterscientificsoftware/bssw-editorial-board, everyone okay with that? If no one has an objection, I can do that ASAP.

Then I can experiment to see if allowing squash on merge of the PR breaks the merge of 'master' to 'preview'.

Related to my epic SEPW-211

bartlettroscoe commented 3 years ago

@bernhold,

One thing that would also greatly reduce problems with merge conflicts on the 'preview' branch is to limit what PRs get merged to 'preview'. We should only be merging PRs that have content that will be directly displayed on the preview.bssw.io site. So PRs that change anything under the docs/ or utils/ directory should never get merged to the 'preview' branch. Likewise, most PRs should wait until some initial reviews before merging to the 'preview' branch and viewing on the preview.bssw.io site. We can accomplish this, as I hinted at 2 years ago in #330 is to require PRs to have the preview label in order to merge to the preview branch. It looks like you might be able to do that with the built-in GHA if feature as described in:

I can experiment with this in my bssw.io GH repo and see how this works.

bernhold commented 3 years ago

@bartlettroscoe

In the action https://github.com/devmasx/merge-branch it supports requiring a label.

bartlettroscoe commented 3 years ago

In the action https://github.com/devmasx/merge-branch it supports requiring a label.

@bernhold, I will take a look.

bartlettroscoe commented 3 years ago

@betterscientificsoftware/bssw-editorial-board ,

Since we did not get a chance to discuss this at the last BSSW EB meeting and with he ECP BOF BSSW session coming up, can we go ahead and implement the protections of the 'preview' branch described above? Any issues with this can be resolved quickly.

Related to this, I also have a version of the merge-pr-to-preview.yml workflow file that looks for a preview label to help avoid the merge conflicts that we have been seeing in files that are never even displayed on preview.bssw.io. I will create a PR for that updated merge-pr-to-preview.yml file along with minimal templates for PRs for the different use cases.

However, the changes to protect the 'preview' branch are not done in a PR, they are done in the settings. But again, these are easy to change.

bartlettroscoe commented 3 years ago

@bernhold, @rinkug, @markcmiller86,

The merge of 'master' to 'preview' is broken once again due to conflicts in files that are not even displayed on the preview.bssw.io site and therefore should never have been merged to the 'preview' branch. These conflicts are:

[rabartl@s1049009 bssw.io (preview)]$ git status
On branch preview
Your branch is up to date with 'github/preview'.

You have unmerged paths.
  (fix conflicts and run "git commit")
  (use "git merge --abort" to abort the merge)

Unmerged paths:
  (use "git add <file>..." to mark resolution)
        both modified:   docs/pages/bssw/bssw_content_publishing.md
        both modified:   docs/pages/bssw/bssw_styling_originalarticles.md

no changes added to commit (use "git add" and/or "git commit -a")

I will fix these conflicts by going with what is on the 'master' branch.

I just posted the new PR #765 that will avoid problems like this going forward because PRs that change files that are never viewed on bssw.io and preview.bssw.io will never be merged to the 'preview' branch and therefore avoid conflicts like this. Can you please review #765 and if you have questions, then let's talk about it.

bartlettroscoe commented 3 years ago

In the action https://github.com/devmasx/merge-branch it supports requiring a label.

@bernhold, FYI, turns out that setting label_name: 'preview' for the devmasx/merge-branch action does not seem to work. I tried that that and it was merging to 'preview' anyway. To get the desired behavior (shown here), I add to add:

if: contains(github.event.pull_request.labels.*.name, 'preview')

which just skips the entire job (which is kind of nice).

I have done detailed manual testing with the updated implementation in PR #765 for the file merge-pr-to-preview.yml. Very small changes but seems to do the trick very well.

bernhold commented 3 years ago

@bartlettroscoe I've reviewed #765. Is there anything else I need to do to move this one along? I can't tell.

bartlettroscoe commented 3 years ago

@bartlettroscoe I've reviewed #765. Is there anything else I need to do to move this one along? I can't tell.

@bernhold, other than merging #765, we also need to protect the 'preview' branch as described above. That is super easy to undo (even temporarily) so there is no risk that I can see in doing this.

bartlettroscoe commented 3 years ago

NOTE: Now that PR #765 has been merged, we need to add the preview label to all of the existing PRs that contain content for the bssw.io site that should be previewed on the preview.bssw.io site. I just created the new preview label for this purpose.

bartlettroscoe commented 3 years ago

FYI: I added the PR checklist and the preview label to open PR #740. Not sure if any other open PR should be be merged to the 'preview' branch.

bartlettroscoe commented 3 years ago

FYI: For the last part of this, I created the team bssw-io-preview-admin and I initially put only @bernhold and myself into it. I then made it so that only this team can push to the 'preview' branch a shown in:

I also allowed force pushes to 'preview' which we may want to do from time to time.

I also noticed that the 'master' branch was completely unprotected. That would allow even forced pushes. Therefore, I added basic branch protect for 'master' so that you can't force push to the branch (which is usually disastrous).

If anyone sees any permission problems with the 'master' or 'preview' branch, please let me know.

I will post a CC article PR ASAP to test that everything works as it should. (And I added a task with subtasks above for these checks.)

bartlettroscoe commented 3 years ago

@bernhold, it looks like PRs from forked repos will never work for the workflow merge-pr-to-preview.yml as they will always result in the error:

https://api.github.com/repos/betterscientificsoftware/bssw.io/merges: 403 - Resource not accessible by integration // See: https://docs.github.com/rest/reference/repos#merge-a-branch (Octokit::Forbidden)

as described in:

This is a major problem for authors who can't push branches to the main bssw.io repo (which is everyone who is not in @betterscientificsoftware/bssw-io-team which means all external contributors).

We are going to have to modify our contribution process to define a "contributor" group that will be allowed to push branches to the main bssw.io repo but not be allowed to do much else. I did this for the https://github.com/TriBITSPub/TriBITS repo for a similar reason.

I don't see any other reasonable solution to this problem.

bernhold commented 3 years ago

Ugh. The pull_request_target trigger was created to make it possible to do stuff from forks. I am sure I had tested this before I put the workflow into bssw.io (from my test repo). Maybe I didn't or maybe they changed something.

I added a manual (workflow_dispatch) trigger to that action. Maybe it will work if someone with permissions triggers the action. That is less automated, but may still be workable.

Do you have a PR handy to try it with? I didn't see one. Or else I can do something small to test.

bartlettroscoe commented 3 years ago

@bernhold,

The pull_request_target trigger was created to make it possible to do stuff from forks.

Yes, but that "stuff" can only involve local testing and checks and may not involve making any modifications back to the main repo. That is the situation described in:

It is because they want to make modifications back to the main repo is where they run into problems.

I added a manual (workflow_dispatch) trigger to that action. Maybe it will work if someone with permissions triggers the action.

We can test that but I don't think that will work. For example, I re-ran the failing GHA jobs and they still failed (even though GitHub should know who I am because I was logged in as me). The problem is that the PR was created from a branch in a repo fork and thus only has read-only access as described in:

which points out:

Note: Workflow runs triggered by Dependabot pull requests run as if they are from a forked repository, and therefore use a read-only GITHUB_TOKEN. These workflow runs cannot access any secrets. See "Keeping your GitHub Actions and workflows secure: Preventing pwn requests" for strategies to keep these workflows secure.

hence, our problem. You can read their justification for this in:

which states:

TL;DR: Combining pull_request_target workflow trigger with an explicit checkout of an untrusted PR is a dangerous practice that may lead to repository compromise.

If you think about it, that makes sense. Anyone on the planet can create a PR with dangerous code and if that can trigger an action that can modify the original repo, you could have a problem. The issue is that GitHub should trust repo maintainers to understand what they are doing and support this. After all, you can do anything you want with a cron job with an account with push access to the GitHub repo. Therefore, GitHub is keeping cron alive!

For example, in our case with bssw.io, you now have to set the label preview and I think only a bssw.io member can set that label, so we have control. (That was not true before the merge of PR #765. Any PR posted from anyone on the planet would trigger that PR-to-preview workflow GHA.)

Do you have a PR handy to try it with? I didn't see one.

I don't have one handy yet.

Based on the above, I don't think we can ever expect a GHA run from a PR created from a fork to push to a branch in the bssw.io repo. So we need a way to get around that limitation. What we need to do to construct the 'preview' branch from 'master' and from the open PRs with the preview label is to use a process that has push access. This idea is hinted at in:

where they use a regular cron job to push things to the repo (in that case, update comments in PR Issues). That would require direct access to the GitHub REST API (but you can use PyGitHub.py for that).

Another option is to have the rebuild process for the preview.bssw.io site create the 'preview' branch locally each time someone manually fires off a rebuild of that site. Then we don't even need to have a 'preview' branch in the GitHub bssw.io repo at all (but we can still have one if we want as that process can force push the updated 'preview' branch). But you need to make sure that there are no conflicts, and if there are, you need a way to resolve them smoothly.

Therefore, we to need flag merge conflicts that would occur when building the 'preview' branch as part of the different GHA triggers. We can still use GHA workflows to test that we can merge the 'master' branch and all of the open PR branches (with the preview label) into a single branch to test for conflicts, we just can't push that locally created branch back to the main bssw.io repo with a GHA. That way, we can ensure that the process that constructs the 'preview' branch can do so without conflicts.

Otherwise, we need to give every author who wants to contribute to bssw.io the ability to push branches to the bssw.io repo. But that would also allow them to possibly mess with other people's branches so that is not a good long-term solution.

You can see why groups are still writing their own GitHub automation using the GitHub REST API and skipping GHA :-(

bernhold commented 3 years ago

@bartlettroscoe thanks for investigating so extensively. I too have looked at these issues before. One thing you need to be sensitive to is that a lot of things you'll uncover date from before GH added the pull_request_target trigger. I need to dig into this deeper because I am pretty certain that I had previously tested exactly what you were trying to do. But it may be that they've further tightened since I tried it. In any case, it looks to me like the pwned article (which is quite recent) may be helpful. I scanned it, and it looked like it had a GHA of triggering a full-permission workflow from a fork PR. It uses an information-passing mechanism similar to that described in the auto_pr_comments thing you linked to. I'm going to be hard-pressed to dig in before Friday or this weekend, probably, though.

bernhold commented 3 years ago

Also, while the of constructing the preview branch only on the server is interesting, the problem is that we'd have no visibility whatsoever in to what was happening with it. If we build it in our repo, we have visibility and control.

bernhold commented 3 years ago

Couldn't help looking at this a little more. Better than writing FWPs. As I read about pull_request_target it says it has write permissions, but runs in the context of the base repo instead of the PR. This comment appears to propose a workaround. I don't know if there is something useful in there for us. We checkout preview and merge the PR head in based on the sha. Need to think more.

bernhold commented 3 years ago

@bartlettroscoe I confirmed that I have successfully merged PRs from forks on my test repository, and I did it again last night. See https://github.com/bernhold/bssw-preview-wf-test-1/pull/51.

I also tried this in bssw.io: https://github.com/betterscientificsoftware/bssw.io/pull/773. Of course I couldn't label it with my unprivileged user. So I labeled it as bernhold and the action generated a merge conflict rather than an permission problem. I haven't figured out the source of the merge conflict yet (not sure how), and I don't think there should be one, but that's different than permissions.

bartlettroscoe commented 3 years ago

@bernhold,

So I labeled it as bernhold and the action generated a merge conflict rather than an permission problem.

GHA jobs fail fast so if there was a conflict, it would report that instead of a permission problem. We need to try it with no merge conflicts to be sure.

Of course I couldn't label it with my unprivileged user

And that is a good thing in our case. It ensures that a bssw.io member has to approve the PR branch to be previewed before anything happens so some evil person can't just post a PR out of nowhere that is designed to do something bad. Requiring a label be set is a safety measure.

There is more discussion of GHA, permissions, and forks in the article:

And looking at:

that is a lot of discussion of the permission issues and PR GHA workflows (but "fork" is only mentioned twice in the latter article).

And then there are options for custom access tokens at the bottom of:

But then you look at pages like:

which says:

Secrets are not passed to workflows that are triggered by a pull request from a fork. ]Learn more](https://docs.github.com/actions/automating-your-workflow-with-github-actions/creating-and-using-encrypted-secrets).

which is very discouraging.

I wish we had access to a GHA workflow expert. They could answer our questions and tell us what was possible in 5 minutes.

bartlettroscoe commented 3 years ago

Also, while the of constructing the preview branch only on the server is interesting, the problem is that we'd have no visibility whatsoever in to what was happening with it. If we build it in our repo, we have visibility and control.

The rebuild page shows some output from the rebuild action. And the point of testing the building of the 'preview' branch with a GHA is that we would know when there are merge conflicts right away so they will not come a a surprise when constructing the 'preview' branch locally on the preview.bssw.io server.

bernhold commented 3 years ago

@bartlettroscoe the biggest point of my earlier comment was the the PR merge succeeded on my test repo. So there is an inconsistency here. I don't think your take-away from the articles you're citing is quite correct. I don't yet fully understand it myself, but, in part, I think you're not paying enough attention to pull_request_target vs pull_request and there are some other associated subtleties here.

bartlettroscoe commented 3 years ago

I think you're not paying enough attention to pull_request_target vs pull_request and there are some other associated subtleties here.

@bernhold, I would agree with you in general but the statement:

Secrets are not passed to workflows that are triggered by a pull request from a fork. Learn more.

at:

suggests that PRs created from a branch from a fork never have write access.

We really need to find someone who is a GHA expert. That would save us hours and hours of investsigation.

bernhold commented 3 years ago

@bartlettroscoe can you please take a look at https://github.com/betterscientificsoftware/bssw.io/pull/736/checks?check_run_id=2248648334 the error is protected branch 'preview' check failed. This happened after I added the preview label to a PR from Kasia from a branch in the bssw.io repo.

I'm assuming this relates to protecting the preview branch, but maybe it is not working as you expected? Because she originated the PR, is this interpreted as her trying to merge to the preview branch? If that's how it works, I don't think this protection will be useful to us.

bartlettroscoe commented 3 years ago

@bernhold, I removed the protections on the 'preview' branch for now so the GHA can merge PR branch to 'preview' branch again. (And you confirmed that the branch in #736 was able to merge to 'preview' just now in https://github.com/betterscientificsoftware/bssw.io/pull/736/checks?check_run_id=2248744473).

This problem is raised in:

which says:

The token will not be able to push to a protected branch as that would enable anyone with write access to your repo to push to that protected branch by simply updating the workflow in a branch. Having that ability would make protected branches useless.

But from looking at this comment it seems that you can provide a personal access token and give that to the GHA. This seems to be further verified in this other comment.

I am wondering if creating and using a personal access token for a GitHub account that has admin right might fix the problem for merging from a fork? Now that I have PR #783 created from a branch in my fork, we can experiment with that some. (But first, I will experiment with this in my own repo for bssw.io.)

bernhold commented 3 years ago

I wonder if https://github.com/marketplace/actions/branch-protection-bot might allow us to maintain the protections most of the time?

bernhold commented 3 years ago

I wonder if https://github.com/marketplace/actions/potential-conflicts-checker might be useful to help identify merge conflicts in advance.

Note that as written, it supports only the pull_request trigger. I think we might need to modify it to support pull_request_target.

There are other conflict detectors in the Marketplace, but most of them seem to use GH's own computation of mergability, and I think that is one PR vs main rather than PRs against each other.

bartlettroscoe commented 3 years ago

There are other conflict detectors in the Marketplace, but most of them seem to use GH's own computation of mergability, and I think that is one PR vs main rather than PRs against each other.

I am a bit surprised that no-one has thought to write such a GHA checker yet. I would be interesting to see what it would take to write such a thing. (I know the GitHub REST API has all the functionality needed to do this.)

bartlettroscoe commented 3 years ago

GHA is driving me crazy. I added the preview label to PR #783 which resulted in the PR-to-preview workflow to fire off with results at:

but it showed a merge conflict showing:

/usr/local/bundle/gems/octokit-4.14.0/lib/octokit/response/raise_error.rb:16:in `on_complete': POST https://api.github.com/repos/betterscientificsoftware/bssw.io/merges: 409 - Merge conflict // See: https://docs.github.com/rest/reference/repos#merge-a-branch (Octokit::Conflict)

I just pulled the updated 'preview' branch locally and merged in the branch from PR #783 and it merged just fine. Since that devmax/merge-branch action provides no feedback at all about what actually conflicted, you have no way to debug this but to try it locally. But I can't find any problem when trying it locally.

GHA is turning into a huge time sink. When the GHA jobs fail, you get very little no useful feedback to try to debug the problem. It seems like you need to write your own fat action that does what you need (by directly calling the GitHub REST API) and then you can provide whatever diagnostics you need.

bartlettroscoe commented 3 years ago

FYI: I created a dummy PR #786 identical to PR #783 instead the one for #786 merged just fine but the one for PR #783 said there was a merge conflict. I wonder which 'preview' branch it is trying to merge to? Is it trying to merge to the 'preview' branch in my fork or in the main bssw.io repo? I could investigate this more and run some experiments to try to reverse engineering this GHA workflow is really doing and see how we might fix this to work correctly from forks.

bartlettroscoe commented 3 years ago

@bernhold, @rinkug

There is no good reason that I can think of to allow rebasing when merging to 'master'. However, if people are sloppy about their commits, it is good to squash when merging the PR. Therefore, we should experiment to see if Git and GitHub will allow this. From a file patch point of view, Git should allow this but we will have to see.

Well, we did that experiment when the commits in PR #783 were squashed when merging that PR to 'master'. That resulted in a merge conflict when trying to merge 'master' to 'preview' shown in:

showing:

/usr/local/bundle/gems/octokit-4.14.0/lib/octokit/response/raise_error.rb:16:in `on_complete': POST https://api.github.com/repos/betterscientificsoftware/bssw.io/merges: 409 - Merge conflict // See: https://docs.github.com/rest/reference/repos#merge-a-branch (Octokit::Conflict)

I verified that manually as documented in https://github.com/betterscientificsoftware/bssw.io/pull/783#issuecomment-812654792.

I will go ahead and disallow squashing or rebasing commits when merging a PR as per the plan above.

bartlettroscoe commented 3 years ago

@bernhold, @rinkug

I will go ahead and disallow squashing or rebasing commits when merging a PR as per the plan above.

I just unchecked the boxes for "Allow squash merging " and "Allow rebase merging" at:

Now when a PR gets merged, it must be a merge commit. That will avoid conflicts when merging from 'master' to 'preview'. (We can allow squash PR merges again once we can rebuild the 'preview' branch from scratch each time.)

bartlettroscoe commented 3 years ago

FYI, the merge from 'master' to 'preview' two days ago failed with:

I just reran that GHA job manually and it passed. Not sure why GHA is so fragile.

bernhold commented 3 years ago

Not sure why GHA is so fragile.

I don't get it either. It seems recent (last few weeks), though perhaps we (the editorial team) are just exercising it more.

bernhold commented 3 years ago

Sorry, but @bernhold needs to generate a notification email to test something.

bartlettroscoe commented 3 years ago

FYI: I am getting consistent failures merging 'master' to 'preview' after the merge of PR #797 as shown at:

But when I do the merge locally, it works just fine. The GHA gives zero clue as to why the merge is failing. This is a terrible system were something fails and it gives no info to debug why. This is the number-1 problem with automated testing systems like this and it can kill a huge amount of productivity when it occurs.

When I write the Python tool to re-create 'preview' from scratch, I will resolve this problem by making sure to print out enough detail to debug problems.

bernhold commented 3 years ago

Yeah, I completely don't understand why we're experiencing so many failures, in situations where I'm pretty sure there really aren't conflicts.

The fault is really with the actions we're using, I think. They throw errors saying there's a merge conflict, but presumably they could be made to provide more details.

From what I've read, I gather GH has some internal algorithm that is uses to compute mergability, and it sounds like it can be complex/time-consuming to run. I know there are some actions that mark PRs as conflicted and all they do is call that function through the GH API, but if they don't wait long enough, it might not complete or the result might not be correct or some such. I wonder if something like that might be going on in these cases -- false negatives due to timeouts or something?

bartlettroscoe commented 3 years ago

@bernhold

FYI: Miracle of miracles but the PR branch from my fork that I just created #823 and put the preview label on was actually merged to the 'preview' branch with no errors! Here is the updated 'preview' branch:

$ git log-short --name-status --graph github/preview

*   c7b0fd3b "Merge 6b5f043f7528934cd753b99acd3011f364bb3777 into preview"
|\  Author: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
| | Date:   Thu May 6 19:20:06 2021 +0000 (5 minutes ago)
| | 
| * 6b5f043f "Make some improvements (#560)"
| | Author: Roscoe A. Bartlett <rabartl@sandia.gov>
| | Date:   Thu May 6 14:56:11 2021 -0400 (11 minutes ago)
| | 
| | M   Articles/HowTos/CriticalBeginnerGitUsageTips.md
| | 
| * 93445d8a "Add meta-data (preview initially) (#560)"
| | Author: Roscoe A. Bartlett <rabartl@sandia.gov>
| | Date:   Thu May 6 14:44:28 2021 -0400 (41 minutes ago)
| | 
| | M   Articles/HowTos/CriticalBeginnerGitUsageTips.md
| | 
| * dbc915f8 "Add new original short article for git beginners tips (#560)"
| | Author: Roscoe A. Bartlett <rabartl@sandia.gov>
| | Date:   Thu May 6 14:36:17 2021 -0400 (49 minutes ago)
| | 
| | A   Articles/HowTos/CriticalBeginnerGitUsageTips.md
.
.
.

So it seems that GHA ran on a PR from a fork can actually modify the base GitHub repo. I am not sure what changed but I am not complaining.

bernhold commented 3 years ago

@bartlettroscoe I've mentioned that in my test repo, I was able to do the preview thing from a fork.

On the other hand, I submitted PR today (#822) which had a merge conflict even though I'm quite confident that there really isn't. I do not understand why this is so unreliable. 😢

bartlettroscoe commented 3 years ago

FYI: I just created #856 to scope out recreating the 'preview' branch from scratch each time.

bartlettroscoe commented 3 years ago

I think we have addressed all of the pressing issues with the 'preview' branch workflow. The next big improvement will will be to create 'preview' from scratch each time and provide better merge-conflict reporting. That will be in the follow-on issue #856.

Therefore, we can close this Issue as "Complete". (Any further problems with the existing 'preview' branch workflow can be handled in small follow-on issues perhaps).

bartlettroscoe commented 3 years ago

FYI: Open Issue #643 considers not allowing direct commits to 'preview'. But as I found above, protecting pushes to 'preview' seems to breaks GitHub Actions jobs that merge and push to 'preview'.