Verify that we can `git push` from the publishing pipeline

f-f commented 3 years ago

Right now we run the API CI pipeline on issues and issue comments - we should verify that from there we are able to authenticate and git push to:

this registry repo itself
and the registry-index repo

The best way to test this would be from some dummy repos, rather than doing it here (i.e. everyone can test this on their own repos)

f-f commented 3 years ago

I think a basic proof of concept of this would be to have:

a workflow that starts from opening an issue
and that commits one file and pushes it to the main branch of the same repo it's contained in

YanikCeulemans commented 3 years ago

As mentioned in the call, I'll do some research on this and report any progress I make in this issue.

YanikCeulemans commented 3 years ago

It seems that there already is an action in the github actions marketplace which pushes to a protected branch using a token and a user name: https://github.com/marketplace/actions/push-to-a-protected-branch This action just runs the following shell script: https://github.com/colmose/push-bot/blob/main/entrypoint.sh

YanikCeulemans commented 3 years ago

There is also https://github.com/marketplace/actions/github-commit-push which combines commit and push using a github token with the added benefit that it supports pushing to a different repo. This would support pushing to the registry-index repo.

YanikCeulemans commented 3 years ago

I think a basic proof of concept of this would be to have:

a workflow that starts from opening an issue

and that commits one file and pushes it to the main branch of the same repo it's contained in

This is now fully functional in my test repo: https://github.com/YanikCeulemans/gh-actions-playground

To proceed we probably want to be able to push to a different repo after doing a git checkout i presume? This would support the second scenario where we would want to push to the registry-index

f-f commented 3 years ago

@YanikCeulemans this is great! :clap:

A constraint that we didn't consider yesterday is the fact that the main branch is protected - I wonder if we can still push to it?

To proceed we probably want to be able to push to a different repo after doing a git checkout i presume? This would support the second scenario where we would want to push to the registry-index

And yes, we should make sure that everything pretty much works before trying to port this approach to here

YanikCeulemans commented 3 years ago

@f-f I've tested pushing to a protected branch and can confirm the push gets rejected.

I've seen in repo settings however that you can allow force pushes to protected branches. If this were setup in the registry repo, we could modify the Github action to use a force push and bypass the protected branch check.

If you would rather not allow force pushes, we would probably have to look for (or create) a Github action which would open a PR containing the changes.

f-f commented 3 years ago

I think we'd be fine with force-pushes from administrators, which is why we are using a token from @pacchettibotti, which is part of the @purescript/packaging team and has access to this

YanikCeulemans commented 3 years ago

@f-f I've enabled the force push option in my gh-actions-playground repo action to test whether or not it would work. It seems that it does not work with the default settings and I'm not completely sure why. My first guess is that the user that the action uses by default:

author_email: github-actions[bot]@users.noreply.github.com
author_name: github-actions[bot]

doesn't have the required permissions to force push to the master branch. The output from the action isn't completely clear on that though:

Run actions-js/push@master
  with:
    github_token: ***
    force: true
    branch: master
    author_email: github-actions[bot]@users.noreply.github.com
    author_name: github-actions[bot]
    directory: .
Started: bash /home/runner/work/_actions/actions-js/push/master/start.sh
Push to branch master
[master 459de74] chore: autopublish 2021-10-03T10:57:08Z
 1 file changed, 1 insertion(+)
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.

I'm going to try and use my email and name for the config options and see if that changes anything. As owner of the repo I can push to the master branch without the --force option.

YanikCeulemans commented 3 years ago

Using my email and name doesn't seem to have helped. I'm not even 100% sure that it actually uses that data and whether or not the force option actually has any effect.

Looking at the source code for the commit and push action, it should use the inputs. The output doesn't show the actual command that was run however.

Maybe the settings for my repo are incorrect, maybe I should use a different github_token like the registry does for the pachettibotti, maybe I should add a similar bot to this repo and make them admin so they can force push?

f-f commented 3 years ago

I think the user and email don't really matter, the token is the part that should really matter auth-wise.

At this point I feel we have two options to make this work:

Disable the "require PR" policy from the repo (this is what we do in package-sets to allow contributors to push the release file directly to trunk):
Have the CI push to a tip branch that is not protected, and then merge it manually to trunk every once in a while.

I think I'd prefer option (2). @thomashoneyman thoughts?

thomashoneyman commented 3 years ago

I'd prefer not to rely on manual merges, and so if these are the only two options then I would choose (1).

There may be a third option: CI pushes to a tip branch that is not protected, the bot opens a pull request, and we use auto-merge to merge the PR? We could use a rule like "CI passes and the PR has a merge label" that the bot applies.

In general I'm against manual intervention except when things go wrong, because essentially it means you and I need to handle it and I've already forgotten to publish package sets releases or push documentation to Pursuit or handle other manual parts of PureScript admin and I'd really rather not add another manual one on top 😆

thomashoneyman commented 3 years ago

Well, on a second read of that article it seems like auto-merge has to be manually applied to open pull requests. So perhaps that doesn't work, but I've at least seen GitHub Actions where a comment triggers a merge and so on -- if we can find some way to keep the main branch protected and require status checks, but not have to manually merge the branch in, that's my ideal solution.

YanikCeulemans commented 3 years ago

This might be possible using the force push strategy discussed above. I haven't been able to get it to work however using the github action from the marketplace. I'll try to copy paste the action into my own repo and run it from there. That way I'll be able to add some debug logging to see what is actually going on. I'll also read up on what the github token actually allows the action to do. I'm assuming it can be used to do a git force push but that might be a false assumption.

I also prefer as much automation as possible so it would be really cool if I could get this to work.

thomashoneyman commented 3 years ago

@YanikCeulemans and I discussed in the registry call that for this to work we'll need to push using the @pacchettibotti personal access token and make sure the account has admin access to both repositories. @YanikCeulemans is going to verify this with the test repository he set up (https://github.com/YanikCeulemans/gh-actions-playground) and then follow up!

jisantuc commented 3 years ago

Have y'all seen merge-me? I think it might be the GitHub action you were looking for with the auto-merge discussion, for example here's a section on automatically merging PRs from a specific bot

YanikCeulemans commented 3 years ago

The approach suggested by @thomashoneyman does actually work. After creating a personal access token and using that instead of the GITHUB_TOKEN the force push action successfully completes.

Run actions-js/push@master
  with:
    github_token: ***
    force: true
    branch: master
    author_email: github-actions[bot]@users.noreply.github.com
    author_name: github-actions[bot]
    directory: .
Started: bash /home/runner/work/_actions/actions-js/push/master/start.sh
Push to branch master
[master a0cf00c] chore: autopublish 2021-10-06T17:16:14Z
 1 file changed, 1 insertion(+)
To https://github.com/YanikCeulemans/gh-actions-playground.git
   e9ee081..a0cf00c  HEAD -> master

This means we won't need to create pull requests thus keeping the flow simple.

thomashoneyman commented 3 years ago

Fantastic news, @YanikCeulemans! And -- just to clarify -- this also works for pushing to other repositories (in our case registry-index and package-sets)?

YanikCeulemans commented 3 years ago

@thomashoneyman, I haven't actually had the chance to test that. Would it suffice that I make a second test repo and try to force push to a protected branch there? Or do I need to test whether a user that is not the owner of a repo (but is admin) can force push to it using their own personal access token (like pachettibotti for the registry and registry index)? In that case I would have to create a second Github account to test it out.

thomashoneyman commented 3 years ago

Maybe I could create a repo and make you an admin?

f-f commented 3 years ago

Great to see that this works! I'm a little concerned about the force-pushing: it two pipelines are running at the same time and starting from the same commit they might risk overwriting each other's commits. The PR approach would always work in this case.

YanikCeulemans commented 3 years ago

In that case we should try to get the action @jisantuc suggested above to work. I'll see what I can do in my own repo and experiment a bit.

YanikCeulemans commented 3 years ago

This might be more complicated than we originally thought. I've looked through the marketplace and haven't really found an off the shelf action or combination of actions which do what I think we need. So I jotted down some notes with all the things that I think need to happen after an issue has been opened:

What do you guys think? I might be way off here but if we do need all of this, it might actually be easier to write a custom Github action that does exactly what we want rather than trying to combine multiple off the shelf ones.

thomashoneyman commented 3 years ago

I'll need to think on this for a sec, but if we do end up needing to write an action, there's a nice library that @colinwahl put together over here: https://github.com/purescript-contrib/purescript-github-actions-toolkit

jisantuc commented 3 years ago

I would expect that the initial workflow would stop at "create PR", and a separate check on PRs would be responsible for merging PRs from the special bot user if CI passes.

Merge conflicts are still pretty tough. We have automated dep upgrades and merge in some of our Scala repos, and git has a pretty tough time with that sometimes, like when whitespace changes on lines collide with automated version upgrades. It's pretty annoying but usually pretty easy to resolve. There's an action for labeling PRs that need conflict resolution here, though that still requires manual intervention from the author... who won't even be the user who opened the issue to kick off the first workflow 🤔, so I'm not sure how they'd be notified. Maybe the registry PR should tag the user who opened the issue, so they're "participating"? (apologies if that's already been decided elsewhere)

thomashoneyman commented 3 years ago

@f-f I know that we need to push to the registry index and to the package sets, but I'm not sure what we need to push to this repository. Would you mind clarifying that?

EDIT: I remember now, it's package metadata. https://github.com/purescript/registry/blob/1c0991529617c386e0d170a8d21a558bec4d5f5e/ci/src/Registry/API.purs#L308-L314

YanikCeulemans commented 3 years ago

Notes from the meeting with Thomas yesterday:

The work needed for this issue is much simpler than the diagram I drew above
We basically need an action to be able to create a PR on this repo. Although I'm not so sure about that now anymore considering the code snippet from the Registry/API module above, which already pushes changes to the master branch.
We also need an action to:
1. checkout a different repo (registry index)
2. create a separate branch
3. make changes
4. push changes
5. create PR

The last bullet point would eventually be implemented in PureScript if it turns out to work properly.

@thomashoneyman please let me know if I misunderstood something.

thomashoneyman commented 3 years ago

Yes, this is about right.

In the CI workflow for this repository we can either a) commit metadata / package set changes directly or b) create a PR with those changes. Either way, we're just doing that with Git commands as part of the Registry.API workflow
In the same workflow (ie. API.yml), we can check out other repositories, make changes, and open pull requests all through normal git commands
In the other repositories (e.g. registry-index), we'll need to write workflows that automatically merge pull requests that are opened by a particular user (ie. @pacchettibotti) and pass all their checks, and maybe something else. Those might require using a GitHub Action or something -- I'm not sure.

Just a quick clarification: I'm distinguishing between workflows, which are the files specifying jobs and steps like our .github/workflows/API.yml file, and actions, which are pre-built tools we can integrate into our workflows (like the checkout or setup-purescript actions)

f-f commented 3 years ago

This all sounds good! I'll add a small note about opening PRs to this repo: we need to consider race conditions, i.e. how do we know that a package is already in the pipeline for being published? (where by "published" we mean "it lands in the trunk branch") If we open a PR for every package being published then we might end up trying to publish the same package more than once if we don't do bookkeeping. If we use only one branch (be it the trunk or some other branch) then the bookkeeping is built in the code that we already have.

f-f commented 2 years ago

Yesterday in the call with @YanikCeulemans we figured that at this point we have explored most of the possibilities for achieving all of this, and it would be a good idea to come up with a detailed plan for implementation.

We settled on something like this:

for pushing to the registry repo: we already have some code here, that ultimately doesn't work because our trunk branch is protected. The way around this is that we would create a new branch for every version published (and reuse the same branch if the pipeline is reattempted!), and automerge it once CI passes (we'll likely have to approve it as well, @pacchettibotti is authorized to do it). The automerge part will have to be a separate workflow. The git part can be implemented in PureScript (as it is now) or as a GitHub action. In this latter case, we'd need to also close the issue at hand on the GitHub action side (it's in PureScript right now). See here where all of this is happening.
for pushing to the registry-index repo: as Yanik's experiment shows, we can push to another repo with GitHub repo with actions. Since registry-index is just a cache, we can just push straight to the main branch, and we are allowed to fail this step. This too can be implemented on the PureScript side or as a GitHub action (likely following the course of action from the above point)

Since this issue's goal was to investigate all of this, we can declare this done and open a new one to track implementation. Thank you for all the effort @YanikCeulemans!

purescript / registry-dev

Verify that we can `git push` from the publishing pipeline #213