(feat): Use existing wrangler installation when appropriate

[AdiRishi]

Resolves #199 #259

Rationale

The code added to this PR has three notable behaviours

1. The installWrangler function now checks for existing wrangler installation. It does so by running the wrangler --version command
2. If the command is successful and the version returned is exactly equal to the required version as per config, the wrangler installation step is skipped
3. If no wrangler version was provided to the config of the action, and a pre-installed wrangler exists, the action will use the pre-installed wrangler
4. The above two steps are wrapped in a try/catch so as to prevent any negative behaviour from this change

The 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 this

 ⛅️ wrangler 3.27.0 (update available 3.28.0)
-------------------------------------------------------

Testing

• I've added a new folder in tests to run this action against a directory with pre-installed wrangler. This test won't fail if something goes wrong, but we can eye it's output to check things work.
• I know the above test method is a bit sub-par. Happy to hear more ideas on how we can test better. There are a few options here, but not sure how detailed we want to go.

[Cherry]

Reducing the time this action takes to run would be awesome!!

• If the command is successful and the version returned is greater than or equal to the required version as per config, the wrangler installation step is skipped

This specific part of this addition concerns me however, as it would be a breaking change.

Today, specifying wranglerVersion in the config of this action is a hard-set version, and not just a semverGt check, and as per the docs:

[...] install a specific version of Wrangler to use for deployment, you can also pass the input wranglerVersion to install a specific version of Wrangler from NPM

As a real-world example, I use the latest wrangler 3.28.x when developing locally, but pin my version in CI to 3.15.0 to workaround secrets issues as per https://github.com/cloudflare/wrangler-action/issues/209. I believe this specific problem is resolved now, but it's not uncommon to want to pin to a specific and reproducible older version in CI, even if that version when developing locally is a little more flexible.

[AdiRishi]

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. Due to the fact that this action pins a "default" wrangler version, I didn't want the behavior to check the pre-installed version against it. That would result in users of this action having to constantly update and keep the wranglerVersion config in sync with whatever they have in package.json

To solve this, I've added some detection in the code to see if a user provided wranglerVersion or if we're using the default.

• If no wrangler version is provided and there exists a pre-installed wrangler in the folder, the action will now use the pre-installed version.
• If a wrangler version was specified, the action will check for an exact version match or else install the requested version

[AdiRishi]

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 CLOUDFLARE_API_TOKEN and CLOUDFLARE_ACCOUNT_ID secrets have been accidentally removed from this repository.

We can see that in the ./run output

 with:
    workingDirectory: ./test/environment
    environment: dev
    preCommands: npx wrangler deploy --env dev
    secrets: SECRET1
  SECRET2

It doesn't list the accountId or apiToken field. What we would expect to see if they were present is something like this

 with:
    accountId: ***
    apiToken: ***

[AdiRishi]

Hello 👋 It's been a while since the last update, just want to bump this. Is anyone looking at this PR?

[petebacondarwin]

Hey @AdiRishi - sorry about the radio silence. We have been rammed preparing for this weeks DevWeek announcements. It will be back to business as usual next week, so I hope we can get someone to move this forward for you then.

[AdiRishi]

@petebacondarwin a gentle bump again on this PR 🙏

[petebacondarwin]

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 CLOUDFLARE_API_TOKEN and CLOUDFLARE_ACCOUNT_ID secrets have been accidentally removed from this repository.

We can see that in the ./run output

 with:
    workingDirectory: ./test/environment
    environment: dev
    preCommands: npx wrangler deploy --env dev
    secrets: SECRET1
  SECRET2

It doesn't list the accountId or apiToken field. What we would expect to see if they were present is something like this

 with:
    accountId: ***
    apiToken: ***

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.

[AdiRishi]

Thank you for the review! Some great points, I'll address them soon.

[petebacondarwin]

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

[AdiRishi]

Understood, I'll take a pass through the existing tests and update them as needed. Also sorry, I clearly forgot to run formatting in my last few commits haha 😅

[AdiRishi]

@petebacondarwin I was looking through the tests, and we clearly reuse ./tests/base and tests/empty a few times. In my mind there are two main ways to tackle this.

1. Explicitly deal with the side effects, e.g rm -rf node_modules explicitly as part of our CI steps.
2. Enforce a convention of having a different folder in tests for each step/check we want to do in CI. So we would have things like tests/only-build, tests/build-quiet etc etc

Imo option one adds more mental overhead when writing and running tests. I think I prefer option two even though it's more duplication it seems like a better convention. What are your thoughts?

[petebacondarwin]

@petebacondarwin I was looking through the tests, and we clearly reuse ./tests/base and tests/empty a few times. In my mind there are two main ways to tackle this.

1. Explicitly deal with the side effects, e.g rm -rf node_modules explicitly as part of our CI steps.
2. Enforce a convention of having a different folder in tests for each step/check we want to do in CI. So we would have things like tests/only-build, tests/build-quiet etc etc

Imo option one adds more mental overhead when writing and running tests. I think I prefer option two even though it's more duplication it seems like a better convention. What are your thoughts?

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.

[AdiRishi]

Alright, gone through all the tests, setup new test folders where necessary. Hope this runs fine in CI 🤞

[AdiRishi]

OK I think we are good here. I'll just try running it on a local branch and then merge. Thanks for the iterations!

Awesome! Will you be pushing these commits to #260 ? I'm keen to see if it runs correctly myself :)