Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Automate checkout, deploy, and --site + --base config #29

Open
wants to merge 28 commits into
base: main
Choose a base branch
from

Conversation

natemoo-re
Copy link
Member

@natemoo-re natemoo-re commented Sep 28, 2023

After looking at how we recommend this action is used, it turns out we can do a bit more to make it simple for people to use. This would be a great v2 for this action.

Based on https://github.com/actions/configure-pages, we can automatically infer the correct --site and --base flags for Astro to use when building. This eliminates the main gotcha with our current setup (that you almost certainly need to update your config with these values).

This also automatically uses actions/checkout@v4 to pull your repo and actions/deploy-pages@v2 to deploy your site to GitHub Pages. These changes mean that our recommended setup is greatly simplified.

name: Deploy to GitHub Pages

on:
  push:
    branches: [main]
  workflow_dispatch:

permissions:
  contents: read
  pages: write
  id-token: write

jobs:
  build:
    runs-on: ubuntu-latest
    environment:
      name: github-pages
      url: ${{ steps.deploy.outputs.url }}
    steps:
      - name: Build and Deploy
        id: deploy
        uses: withastro/action@configure-site
        # when this is released, it will be withastro/action@v2

Instead of running npm run build, we now run the astro CLI directly. Honestly this is the change I'm most hesitant about, because some folks might want CI to fail if astro-check fails or use pre/post-build scripts. This change would skip those and just do astro build. Open to feedback!

Copy link

@KorigamiK KorigamiK left a comment

Choose a reason for hiding this comment

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

lgtm!

@KorigamiK
Copy link

If this is working maybe you look into merging?

action.yml Outdated Show resolved Hide resolved
action.yml Outdated Show resolved Hide resolved
action.yml Outdated Show resolved Hide resolved
action.yml Outdated Show resolved Hide resolved
@natemoo-re
Copy link
Member Author

@KorigamiK If you want to try this PR while we're working on it, you're welcome to point your repository to it! Using withastro/action@configure-site should do the trick.

@natemoo-re natemoo-re changed the title Automatically configure site and base Automate checkout, deploy, and --site + --base config Oct 6, 2023
Copy link
Member

@delucis delucis left a comment

Choose a reason for hiding this comment

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

I think this is great!

One thing I’ll say regarding automatic base is that users probably still want to set base in their config in most cases. Otherwise while developing locally you have a base of / and links using /github-base/ will 404. Saw this confusion in withastro/starlight#804 for example where a user deploying to GH Pages had manually set:

base: process.env.CI ? '/github-base/' : undefined,

making it impossible to get links working in both places.

I don’t know if that makes setting base automatically less attractive or not. Would still work well for people who mostly don’t work locally.

action.yml Outdated
Comment on lines 112 to 113
$RUNNER astro build \
--site "${{ steps.pages.outputs.origin }}" \
--base "${{ steps.pages.outputs.base_path }}"
Copy link
Member

Choose a reason for hiding this comment

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

I can understand the hesitation here is it makes customisation of the build step impossible (no way to run pre/post scripts as well for example).

I guess adding an input that tells us the build command risks not working with automatic --site and --base flags if someone’s package script is not the right form? (e.g. something like "build": "astro build && node ./post-build")

Some options off the top of my head:

  • Add prebuild/postbuild inputs for scripts to run either side of the build
  • Add default steps that do this “automatically” (npm run prebuild --if-present — maybe no Yarn support?)
  • Just add a single build command input that overrides our clever automatic one — up to a user to configure site/base correctly if they decide to override for some reason.

Copy link
Member Author

@natemoo-re natemoo-re Oct 6, 2023

Choose a reason for hiding this comment

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

Yeah I was checking how Vercel does this, and they phrase it as "overrides," which makes sense. actions/configure-pages also has an interesting approach where they override the values in the config file after cloning. So that's maybe a more fool-proof method.

Of course, if Astro accepted ASTRO_SITE and ASTRO_BASE env variables, that could work too. Or we could just publish a GitHub Pages integration that automatically wires those two values for you?

Many ways to tackle the problem!

natemoo-re and others added 9 commits January 3, 2024 12:45
Based on https://github.com/actions/configure-pages, automatically infer the correct `--site` and `--base` flags for Astro to use when building.

Also adjusts `npm install` to use `npm ci` and switches away from using the default `build` script to instead use the `npx --no-install astro build`. Unsure if this is a good idea.
@ollegkz
Copy link

ollegkz commented Jan 3, 2024

Looking forward to test this @natemoo-re 💯 what did I miss here? https://github.com/ollegkz/olle.coffee/actions/runs/7401803673/job/20138353320

@natemoo-re
Copy link
Member Author

Looking forward to test this @natemoo-re 💯 what did I miss here? https://github.com/ollegkz/olle.coffee/actions/runs/7401803673/job/20138353320

That is probably on my end! I'm guessing the command is not correct so Bun is printing its help message instead

@ollegkz
Copy link

ollegkz commented Jan 5, 2024

Seems to work as of 6e05a33 as you can see https://github.com/ollegkz/olle.coffee/actions/runs/7423744689/job/20201840905 but the url is no longer printed out in the deploy complete step 😮

Compare https://github.com/ollegkz/olle.coffee/actions/runs/7423744689/job/20201840905 complete job with https://github.com/ollegkz/olle.coffee/actions/runs/7400679346/job/20134828466 :suprised_pikachu:

@natemoo-re
Copy link
Member Author

Thanks @ollegkz! I forked your repo so I can test changes directly against it. I think it's bun-specific.

@ollegkz
Copy link

ollegkz commented Jan 10, 2024

Any luck on merging this @natemoo-re? 🥇

@swift502
Copy link
Contributor

swift502 commented Jan 10, 2024

Works for root deployment.
And base works too. 🙏

To summarize then, the "site" param in astro.config.js is no longer mandatory for deployment?
But base still has a few gotchas, if deploying with a base:

  • Internal links still have to be updated to the new base before deployment
  • If you want to astro dev sites with internal links, you still have to define a base in your config

But I hope we can simplify the docs a bit once this is merged.

@ollegkz
Copy link

ollegkz commented Jan 23, 2024

Hmm, I just noticed that actions/configure-pages@v3 uses node16 and actions/configure-pages@v4 uses node20 :)

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.

None yet

5 participants