ci: use github action for automated deployment - #1337

Cool, beat me to it. Here is what I was working on locally, because there are probably some bits you might want

name: PR to official images

on:
  pull_request_target:
    types:
      - closed
    paths:
      - "**/Dockerfile"
      - "**/architectures"
      - "**/docker-entrypoint.sh"
      - "generate-stackbrew-library.sh"
      - "functions.sh"

jobs:
  pr:
    runs-on: ubuntu-latest
    if: github.repository_owner == 'nodejs' && github.event.pull_request.merged_by != ''

    steps:
      - name: Checkout the docker-node repo
        uses: actions/checkout@v2
        with:
          path: docker-node
          fetch-depth: 50

      - name: Checkout the docker-library/official-images repo
        uses: actions/checkout@v2
        with:
          path: official-images
          repository: docker-library/official-images

      - name: Generate Stackbrew for diff
        run: |
          cd docker-node
          ./generate-stackbrew-library.sh > ../official-images/library/node

      - uses: peter-evans/create-pull-request@v3
        with:
          token: ${{ secrets.GITHUB_API_TOKEN }}
          push-to-fork: nodejs-github-bot/official-images
          title: "Node: ${{ github.event.pull_request.title}}"
          path: official-images
          commit-message: "Pull Request: {{ github.event.pull_request.html_url }}"

I have extra complexity in mine that handles if commits are pushed to master without going through a pull request. Should I assume that won't happen and take out some of that stuff?

The main branch is protected, so it can only be changed through PRs. That should simplify some stuff for you 😄

@nschonni I see in your approach, you are using the pull_request_target trigger. It seems like it does have some benefits in that you have access to the PR without having to search for it. I changed over to that method and added the other differences too.

@nodejs/docker (or maybe specifically @LaurentGoderre) does anyone have the token that we used for Travis that they could put in the GitHub secrets section?

@nodejs/build-infra could you add the bot's token to the repo's secrets? Or is that stuff managed by some other group?

Created nodejs/admin#556 since it seems like there is more of an official process now around the tokens and secrets

docker_node_github_api_token.md is in build/github-bot in the secrets repo already, I assume that's what you're asking for?

@rvagg yup, that would work! We're just looking to switch the CI the PR setup is running on

Still need approval on the admin repo. I can setup the secrets on this repo when request is approved.

One more question, is it possible to not run this if tests or building an image fail? Or is the thinking that since this only runs on master (which is protected), CI was green before merging so we don't need to wait.

Looking at https://docs.github.com/en/free-pro-team@latest/actions/reference/workflow-syntax-for-github-actions#jobsjob_idneeds it doesn't seem like the ID is referencable across workflows. And we wouldn't know which to wait for either as builds are triggered conditionally. I guess we could try to have one workflow instead of many? Not sure how that would look

The deployment steps would need to be on the same job as the tests so if a test fails this job doesn't run, or the job has to be in the same file and can depend on the test/build job.

The deployment steps would need to be on the same job as the tests so if a test fails this job doesn't run, or the job has to be in the same file and can depend on the test/build job.

I like this way too. Another way is to use some an action like lewagon/wait-on-check-action to wait for a test workflow to finish before allowing another one to run.

@rvagg we got the thumbs up over on the admin repo nodejs/admin#556 (comment) if you have access to set the secret

no, I don't think I have all the required access, @mmarchini might be the best person for this?

From @yosifkit docker-library/official-images#8824 (comment)

I think we'd rather that the bot be improved to only open a single PR at a time.

Maybe we should drop the branch suffix and just keep a single "official-images" branch that it could keep pushing to the same PR till landed

I think we're good to land this, just wanted to walk through the flow now that we're re-using the branch

Flow 1:

1. PR A lands here
2. Action creates upstream PR ##
3. Action comments on PR A
4. Upstream PR lands. Branch is leftover or deleted?

Flow 2:

1. PR A lands here
2. Action creates upstream PR ##
3. Action comments on PR A
4. PR B lands here
5. Action pushes new commits. Title stays the same?
6. Action comments on PR B
7. Upstream PR lands. Branch is leftover or deleted?

Flow 3:

1. PR A lands here
2. Action creates upstream PR ##
3. Action comments on PR A
4. Upstream PR is rejected/closed. Branch is leftover or deleted?

The action uses peter-evans/create-pull-request, and they describe their behavior like: https://github.com/peter-evans/create-pull-request#action-behaviour.

In Flow 1, currently the branch is NOT deleted because delete-branch is not specified and defaults to false. This can be changed if you want.

In Flow 2, step 5, the PR title will be updated (so will the PR body). Step 7, branch is currently not deleted because of what I mentioned above.

In Flow 3, branch is not deleted for same reason.

I think it may be a good idea to set delete-branch to true because it will cause the branch to be deleted in all 3 flows, which seems desirable.

I can't tell from the docs if there would need to be some sort of repository_dispatch setup on the other side for the delete branch to work, so I'm good leaving it for now.
I think there maybe be an issue with the fact we're using the base action/checkout then using the PR action to re-use/update the branch, but unless anyone has any last minute changes, I think we should land this and then test it out by landing #1347 then #1348

I can't tell from the docs if there would need to be some sort of repository_dispatch setup on the other side for the delete branch to work, so I'm good leaving it for now.
I think there maybe be an issue with the fact we're using the base action/checkout then using the PR action to re-use/update the branch, but unless anyone has any last minute changes, I think we should land this and then test it out by landing #1347 then #1348

If delete-branch is true, it appears that the action would delete the branch the next time it is invoked on this repository (and there is no diff). https://github.com/peter-evans/create-pull-request/blob/e17bb55cb71e482dbeb01f204bff23f21dd20c77/src/create-pull-request.ts#L204

Overall, I now agree with you and we probably don't need to enable it.

As for wondering if there could be conflicts, it pushes to the branch with this command: https://github.com/peter-evans/create-pull-request/blob/e17bb55cb71e482dbeb01f204bff23f21dd20c77/src/create-pull-request.ts#L179

await git.push([
  '--force-with-lease',
  branchRemoteName,
  `HEAD:refs/heads/${inputs.branch}`
])

OK, I'm going to land this and the other 2 and we'll see how it goes!
Thanks for all the work

@mmarchini FYI, we did run into the merge issue with the first PR using the pull_request_target, so I guess the regular merge isn't the workaround and we'd need to switch the strategy to rebase merge

@mmarchini FYI, we did run into the merge issue with the first PR using the pull_request_target, so I guess the regular merge isn't the workaround and we'd need to switch the strategy to rebase merge

Yeah I messed up in my initial testing of the trigger. When I run the tests again, its behaving the same way it's behaving in this repo now.