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

Add GitHub deployments for parity with pages-action #325

Conversation

Maximo-Guk
Copy link
Member

@Maximo-Guk Maximo-Guk commented Nov 13, 2024

Addresses #301

This PR updates wrangler-action dependencies and adds @actions/github, so that we can create GitHub deployments, as well as job summaries when a user provides their GiHub token to the action config.

Github deployments successfully tested here:
image

Job summary successfully tested here
image

@Maximo-Guk Maximo-Guk requested review from a team as code owners November 13, 2024 22:24
@@ -14,6 +14,43 @@ const OutputEntryPagesDeployment = OutputEntryBase.merge(
url: z.string().optional(),
alias: z.string().optional(),
environment: z.enum(["production", "preview"]),
// optional, added in wrangler@TBD
Copy link
Member Author

@Maximo-Guk Maximo-Guk Nov 13, 2024

Choose a reason for hiding this comment

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

Fill this out when there's been a new wrangler release that has the new fields we need for pages deploy detailed artifact

@Maximo-Guk Maximo-Guk force-pushed the maximo/add-github-deployments-parity-for-pages-deployments-in-wrangler-action branch from 027ebd5 to c7edd46 Compare November 13, 2024 22:35
@Maximo-Guk
Copy link
Member Author

Maximo-Guk commented Nov 13, 2024

Do not merge until:

  • wrangler has been released with new artifact fields

src/wranglerAction.ts Outdated Show resolved Hide resolved
src/wranglerAction.ts Outdated Show resolved Hide resolved
src/github.ts Outdated Show resolved Hide resolved
src/github.ts Outdated Show resolved Hide resolved
@Maximo-Guk Maximo-Guk linked an issue Nov 13, 2024 that may be closed by this pull request
@Maximo-Guk Maximo-Guk force-pushed the maximo/add-github-deployments-parity-for-pages-deployments-in-wrangler-action branch 4 times, most recently from 08b3cf6 to ef98565 Compare November 18, 2024 18:51
@Maximo-Guk Maximo-Guk force-pushed the maximo/add-github-deployments-parity-for-pages-deployments-in-wrangler-action branch from ef98565 to ec43b38 Compare November 18, 2024 19:23
@courtney-sims
Copy link
Contributor

I like having all the github logic into one file!

@Maximo-Guk Maximo-Guk force-pushed the maximo/add-github-deployments-parity-for-pages-deployments-in-wrangler-action branch 4 times, most recently from 684a156 to 7035fbc Compare November 18, 2024 22:24
@aaronadamsCA
Copy link

Great to see this happening!

I contributed some of the Pages action's features, and I have a few suggestions to avoid repeating some of its mistakes:

  • Don't require GitHub token as an action input. Default its value to github.token (see actions/checkout for a great example), then let users turn features on/off using boolean inputs. This is a lot less confusing and aligns with the rest of the ecosystem.

  • Accept environment name as input. Named GitHub environments can hold configurations, so migrating users might need this action's environment names to match their old ones, so that those configurations will still apply. And one repo can deploy multiple Wrangler apps, so their environment names must be configurable or else they will collide.

  • Provide separate inputs to enable deployments vs. job summaries. Deployments add a lot of noise on the Conversation tab, whereas job summaries add a lot of noise on the Checks tab. Ideally users could choose how much noise they want.

@Maximo-Guk Maximo-Guk force-pushed the maximo/add-github-deployments-parity-for-pages-deployments-in-wrangler-action branch from 7035fbc to e87d79c Compare November 19, 2024 17:38
@Maximo-Guk Maximo-Guk force-pushed the maximo/add-github-deployments-parity-for-pages-deployments-in-wrangler-action branch from e87d79c to cada7a6 Compare November 19, 2024 18:08
Copy link
Contributor

@jahands jahands left a comment

Choose a reason for hiding this comment

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

LGTM!

@Maximo-Guk
Copy link
Member Author

Great to see this happening!

I contributed some of the Pages action's features, and I have a few suggestions to avoid repeating some of its mistakes:

  • Don't require GitHub token as an action input. Default its value to github.token (see actions/checkout for a great example), then let users turn features on/off using boolean inputs. This is a lot less confusing and aligns with the rest of the ecosystem.
  • Accept environment name as input. Named GitHub environments can hold configurations, so migrating users might need this action's environment names to match their old ones, so that those configurations will still apply. And one repo can deploy multiple Wrangler apps, so their environment names must be configurable or else they will collide.
  • Provide separate inputs to enable deployments vs. job summaries. Deployments add a lot of noise on the Conversation tab, whereas job summaries add a lot of noise on the Checks tab. Ideally users could choose how much noise they want.

Thanks a lot Aaron! I'm going to implement those changes in a separate PR as this PR is already getting to be pretty big!

@Maximo-Guk Maximo-Guk merged commit b19342b into main Nov 21, 2024
4 checks passed
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.

[pages-action migration] Add Support for Deployments Feature
4 participants