-
Notifications
You must be signed in to change notification settings - Fork 163
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
(feat): Use existing wrangler installation when appropriate #235
Conversation
3fa14d7
to
0cf9a76
Compare
Reducing the time this action takes to run would be awesome!!
This specific part of this addition concerns me however, as it would be a breaking change. Today, specifying
As a real-world example, I use the latest wrangler |
0cf9a76
to
1dadfd3
Compare
Apologies for the delay in responding, got busy with something else. @Cherry thanks for the comments, I didn't consider the exact version requirements for wrangler. To solve this, I've added some detection in the code to see if a user provided
|
Hmm, I looked into the test failure. It seems unrelated to my change? From looking through the code and the action output I would say that the We can see that in the
It doesn't list the
|
Hello 👋 |
Hey @AdiRishi - sorry about the radio silence. We have been rammed preparing for this weeks DevWeek announcements. |
@petebacondarwin a gentle bump again on this PR 🙏 |
We can't run the full tests on a PR that comes from a 3rd party fork of the repository. Github Actions blocks sharing secrets with 3rd parties. |
"license": "MIT", | ||
"private": true, | ||
"devDependencies": { | ||
"wrangler": "^3.28.0" |
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.
Since this is a test of checking to see if we can use a preinstalled version then it would make sense (with or without a package-lock file) to make this an explicit version.
"wrangler": "^3.28.0" | |
"wrangler": "3.28.0" |
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.
Actually how about we do something really crazy and build a fake wrangler into a node_modules
directory here that responds to the version
command with a hardcoded version and also supports some fake Wrangler command such as test-wrangler-action
. If we end up installing a different version over this then running that command would fail.
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.
That is truely a wild idea 🤯
Any ideas on how to do something like this? The only thing that comes to my mind is something like patch-package.
Would love to know your thoughts.
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.
Nothing so clever. I can push a commit for you to consider if you like
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 pushed a commit but it didn't quite work in the CI as hoped. I can check again later today.
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.
Very interesting idea, I understand the path you were suggesting now. I've pushed a minor fix that I think will help its run in CI. Its working locally for me at least when I run npx wrangler action-test
.
I've also made a fix for the other comment about simplifying the code structure for the if/else logic
Thank you for the review! Some great points, I'll address them soon. |
9bf6e6c
to
de01fe0
Compare
de01fe0
to
06f5e60
Compare
OK things are looking good. Now that there is logic to reuse current installations of Wrangler, we need to be careful to remove any from tests to avoid leaking between them. E.g. https://github.com/cloudflare/wrangler-action/actions/runs/8970219683/job/24633372708#step:13:17 |
Understood, I'll take a pass through the existing tests and update them as needed. |
@petebacondarwin I was looking through the tests, and we clearly reuse
Imo option one adds more mental overhead when writing and running tests. |
I think I agree with you in this case. There is a trade-off if the cost of setting up an environment incerases which can cause CI jobs to slow down, but I think we are a long way off here. |
Alright, gone through all the tests, setup new test folders where necessary. Hope this runs fine in CI 🤞 |
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.
OK I think we are good here. I'll just try running it on a local branch and then merge.
Thanks for the iterations!
@@ -52,28 +51,20 @@ jobs: | |||
accountId: ${{ secrets.CLOUDFLARE_ACCOUNT_ID }} | |||
environment: dev | |||
preCommands: npx wrangler deploy --env dev # https://github.com/cloudflare/wrangler-action/issues/162 | |||
postCommands: npx wrangler delete --name wrangler-action-dev-environment-test --force |
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.
Hmm. I think this is OK. But what happens if this step fails?
We should probably implement a cleanup job that runs on a schedule similar to workers-sdk e2e test cleanup...
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.
Yeah that sounds like something we should add. Do you want me to do it in this PR? If not I'm happy to make another PR after this one to add it in.
workingDirectory: "./test/base" | ||
apiToken: ${{ secrets.CLOUDFLARE_API_TOKEN }} | ||
accountId: ${{ secrets.CLOUDFLARE_ACCOUNT_ID }} | ||
command: delete --name wrangler-action-dev-environment-test --force |
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.
Is this actually a test we want to do? i.e. run the delete command?
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.
That's a great question. We actually do have a delete
command below when cleaing up the secret test workers. I figured this change along with another one down below would give us the chance to test out postCommands
. Since there wasn't a previous use of it in this script.
Awesome! Will you be pushing these commits to #260 ? I'm keen to see if it runs correctly myself :) |
…loudflare#235)" This reverts commit 0545ad2.
…loudflare#235)" This reverts commit 0545ad2.
Revert "(feat): Use existing wrangler installation when appropriate #235"
…priate (cloudflare#235)"" This reverts commit 2d275a8.
…priate (cloudflare#235)"" This reverts commit 2d275a8.
Unreverts #235 and don't automatically install wrangler when checking if it present
Resolves #199 #259
Rationale
The code added to this PR has three notable behaviours
installWrangler
function now checks for existing wrangler installation. It does so by running thewrangler --version
commandexactly equal
to the required version as per config, the wrangler installation step is skippedtry/catch
so as to prevent any negative behaviour from this changeThe decision to check versions seems sound to me, as it is entirely conceivable that users of this library want to install a newer wrangler version in CI than the pinned version in their repositories.
Just to remind everyone,
wrangler --version
output can look like thisTesting