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

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

Merged
merged 2 commits into from
Jun 20, 2024

Conversation

Maximo-Guk
Copy link
Member

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

@Maximo-Guk Maximo-Guk self-assigned this Jun 16, 2024
@AdiRishi
Copy link
Contributor

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

Copy link
Contributor

@AdiRishi AdiRishi left a comment

Choose a reason for hiding this comment

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

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.

@Maximo-Guk Maximo-Guk marked this pull request as ready for review June 20, 2024 15:42
@Maximo-Guk Maximo-Guk requested review from a team as code owners June 20, 2024 15:43
@Maximo-Guk
Copy link
Member Author

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

@Maximo-Guk Maximo-Guk merged commit 8f2f895 into main Jun 20, 2024
1 of 2 checks passed
@github-actions github-actions bot mentioned this pull request Jun 20, 2024
@Maximo-Guk Maximo-Guk deleted the maximo/fixup-235 branch June 20, 2024 21:45
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