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
Support wrangler2 - v2 #72
Support wrangler2 - v2 #72
Conversation
LGTM! 🙏 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PCX review
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice update @WalshyDev - I think the action should still support setting the environment.
README.md
Outdated
with: | ||
apiToken: ${{ secrets.CF_API_TOKEN }} | ||
command: publish | ||
``` | ||
|
||
If you need help defining the correct cron syntax, check out [crontab.guru](https://crontab.guru/), which provides a friendly user interface for validating your cron schedule. | ||
|
||
### Deploying on a "dispatched" event |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that mentioning dispatch event here at all is an implementation concern.
Instead what the user wants to do here is trigger a deploy manually...
### Deploying on a "dispatched" event | |
### Manually triggering a deployment |
00dd53d
to
5c36a64
Compare
5c36a64
to
3c8fc21
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Left that one suggestion for the docs, but not-blocking, imo.
I'd wait for one or two more people to approve, and then we can merge!
By the way, I don't have write access on this repo so I cannot merge or cut a release. @codewithkristian or @eidam would probably need to |
c42b80c
to
3d1979c
Compare
To deploy static sites and frontend applications to Workers, check out the documentation for [Workers Sites](https://developers.cloudflare.com/workers/sites). | ||
|
||
Note that this action makes no assumptions about _how_ your project is built! **If you need to run a pre-publish step, like building your application, you need to specify a build step in your Workflow.** For instance, if I have an NPM command called `build`, my workflow YAML might resemble the following: | ||
You will need to add `account_id = ""` in your `wrangler.toml` file or set `accountId` in this GitHub Action. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Improved the error message around this.
I can merge it in 30
…On Mon, May 9, 2022 at 11:22 Daniel Walsh ***@***.***> wrote:
By the way, I don't have write access on this repo so I cannot merge or
cut a release.
@codewithkristian <https://github.com/codewithkristian> or @eidam
<https://github.com/eidam> would probably need to
—
Reply to this email directly, view it on GitHub
<#72 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AA3NDLG2XHFNRYU6WPSP633VJDRQFANCNFSM5VF4HI5Q>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
Whooo! Wrangler 2 is awesome :)
Since this is v2 I did make 2 breaking changes but the migration path is super simple.
publish: true
withcommand: publish
.You can see it working for a Worker publish here: https://github.com/WalshyDev/wrangler-action-test/runs/6310845616?check_suite_focus=true
You can see it working for a Pages deployment here: https://github.com/WalshyDev/wrangler-action-test/runs/6327883564?check_suite_focus=true
cc: @GregBrimble @eidam @Ekwuno