search

cloudflare / wrangler-action

🧙‍♀️ easily deploy cloudflare workers applications using wrangler and github actions
Apache License 2.0
1.27k stars 160 forks source link

Unreverts #235 and don't automatically install wrangler when checking if it present #271

Closed Maximo-Guk closed 5 months ago

Maximo-Guk commented 5 months ago

See !239 for more context

This PR adds the execNoInstall field to packageManagers, so that when we're executing wrangler to check if it present with our various package manager configurations, we don't automatically install it, in the case that it isn't present.

This was tested by running test-fixup-235 branch on the test-action branch of my demo-actions repo which contains hello-world workers applications for npm, pnpm, yarn and bun.

The successful pipeline where each package manager was tested https://github.com/Maximo-Guk/demo-actions/actions/runs/9533218220

cc @AdiRishi

AdiRishi commented 5 months ago

@Maximo-Guk as I said in the comment in my PR, this approach is much better. I like how you solved it 🚀

Maximo-Guk commented 5 months ago

The only thing I'll add is that it might be good to add a test to ensure it installs the correct version of wrangler by default. Could be a test we add directly into deploy.yml, perhaps a simple exection of wrangler --version and checking that it returns the exected output.

I'd really like to add a test for this, however with the current state of tests in this repo #194 I'm going to hold off on that for now, and get this merged in relying on the manual tests.

I'm hoping to work on converting most of the e2e deploy.yaml tests to proper integration tests sometime soon