Skip to content

Conversation

@crazy-max
Copy link
Member

@crazy-max crazy-max commented May 22, 2022

follow-up #14685

Refactor and mutualize our local actions in https://github.com/docker/docker.github.io/tree/master/.github/actions into a single one called releaser. this one is containerized so it can also been tested locally with buildx bake and doesn't require a GitHub Runner.

netlify-deploy and netlify-clean has been refactored and is using now the official Go library instead of the npm CLI so we are aligned with the AWS one that updates the website config and used the AWS Go SDK.

update-website-config had been refactored a bit and is now available as a subcommand of releaser app: releaser aws update-config. It's also updated to a more recent AWS Go SDK. In a follow-up we should update to the AWS SDK v2.

# update website config (staging)
$ AWS_REGION=us-east-1 \
  AWS_S3_BUCKET=docs.docker.com-stage-us-east-1 \
  AWS_S3_WEBSITE_CONFIG=_website-config-docs-stage.json \
  AWS_ACCESS_KEY_ID=foo \
  AWS_SECRET_ACCESS_KEY=bar \
  docker buildx bake aws-update-config
# remove site from Netlify
$ NETLIFY_AUTH_TOKEN=foo \
  NETLIFY_SITE_NAME=docker/docker.github.io/master \
  docker buildx bake netlify-remove

@netlify
Copy link

netlify bot commented May 22, 2022

Deploy Preview for docsdocker ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit 3405170
🔍 Latest deploy log https://app.netlify.com/sites/docsdocker/deploys/62d54a962c1f720008dd2b39
😎 Deploy Preview https://deploy-preview-14803--docsdocker.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

Comment on lines -48 to -62
# Disabled netlify-deploy due to flakey 502 http errors
# - name: copy static files
# if: github.event.pull_request.head.repo.fork == false
# run: docker run -v ${PWD}:/output documentation:latest cp -r /usr/share/nginx/html /output/_site
# - uses: ./.github/actions/netlify-deploy
# if: github.event.pull_request.head.repo.fork == false
# with:
# directory: _site
# netlify_token: ${{ secrets.NETLIFY_AUTH_TOKEN }}
# netlify_account_slug: ${{ secrets.NETLIFY_ACCOUNT_SLUG }}
# site_name: "${{ github.repository }}/${{ github.head_ref }}"
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With this refactoring we might enable this workflow and see if it works fine now but don't think it's necessary to have it anymore if we want to only accept pull requests from forks and disable auto publish. Step would look like this:

      -
        name: Netlify deploy
        if: github.event.pull_request.head.repo.fork == false
        uses: docker/bake-action@v2
        with:
          targets: netlify-deploy
        env:
          NETLIFY_AUTH_TOKEN: ${{ secrets.NETLIFY_AUTH_TOKEN }}
          NETLIFY_ACCOUNT_SLUG: ${{ secrets.NETLIFY_ACCOUNT_SLUG }}
          NETLIFY_SITE_NAME: ${{ github.repository }}/${{ github.head_ref }}
          NETLIFY_PUBLISH_DIR: "_site"

Comment on lines 1 to 27
name: clean-pr

on:
pull_request:
types:
- closed

jobs:
remove-site-from-netlify:
name: build
runs-on: ubuntu-18.04
netlify-remove:
runs-on: ubuntu-20.04
if: github.event.pull_request.head.repo.fork == false
steps:
- uses: actions/checkout@v2
-
name: Checkout
uses: actions/checkout@v3
with:
ref: ${{ github.event.pull_request.head.sha }}
- name: delete from netlify
uses: ./.github/actions/netlify-clean
-
name: Remove site from Netlify
uses: docker/bake-action@v2
with:
netlify_token: ${{ secrets.NETLIFY_AUTH_TOKEN }}
site_name: "${{ github.repository }}/${{ github.head_ref }}"
targets: netlify-remove
env:
NETLIFY_AUTH_TOKEN: ${{ secrets.NETLIFY_AUTH_TOKEN }}
NETLIFY_SITE_NAME: ${{ github.repository }}/${{ github.head_ref }}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at the remove published site from netlify workflow, I think we could remove it in the future if we only accept pull request from forks as we already discussed. And we could disable the auto publish feature from master branch on Netlify too, so we would not need this workflow at all:

image

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And we could disable the auto publish feature from master branch on Netlify too, so we would not need this workflow at all:

Is that publish needed to get a preview for the "update published from master" PRs, such as #15128?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't think it's needed anymore as we already deploy master branch in our publish workflow

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh you mean the "published" branch in #15128. Yes it might be relevant if we don't restrict to only pull request. IMO netlify makes sense only on PR.

@crazy-max crazy-max force-pushed the netlify-cli-and-aws branch from da25fd2 to 4631035 Compare July 14, 2022 17:30
@crazy-max crazy-max marked this pull request as ready for review July 15, 2022 19:04
@crazy-max
Copy link
Member Author

crazy-max commented Jul 15, 2022

This PR is needed to put in place the lambda redirects function tested in crazy-max@d44d840

If you can PTAL @thaJeztah

Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

overall this looks good; perhaps squash the commits? (as the first commit adds the replacement code that's removed in the second one)

Left some random comments

@crazy-max crazy-max force-pushed the netlify-cli-and-aws branch 2 times, most recently from 6f1ca93 to 1674948 Compare July 15, 2022 19:44
@crazy-max crazy-max requested a review from thaJeztah July 15, 2022 19:50
@crazy-max crazy-max force-pushed the netlify-cli-and-aws branch from 1674948 to 4bdb2f3 Compare July 15, 2022 19:52
@crazy-max
Copy link
Member Author

perhaps squash the commits? (as the first commit adds the replacement code that's removed in the second one)

@thaJeztah Done

@crazy-max crazy-max force-pushed the netlify-cli-and-aws branch from 4bdb2f3 to 3a33cdf Compare July 17, 2022 23:03
Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

perhaps would be good to have another set of eyes on this though

@crazy-max crazy-max force-pushed the netlify-cli-and-aws branch 2 times, most recently from f1894b4 to 962b4f2 Compare July 18, 2022 10:20
Signed-off-by: CrazyMax <crazy-max@users.noreply.github.com>
@crazy-max crazy-max force-pushed the netlify-cli-and-aws branch from 962b4f2 to 3405170 Compare July 18, 2022 11:57
@crazy-max
Copy link
Member Author

perhaps would be good to have another set of eyes on this though

Could you PTAL @StefanScherer?

Copy link
Member

@StefanScherer StefanScherer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Thank you.

@crazy-max crazy-max merged commit 2819c9d into docker:master Jul 18, 2022
@crazy-max crazy-max deleted the netlify-cli-and-aws branch July 18, 2022 16:54
@crazy-max crazy-max mentioned this pull request Jul 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants