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

(feat): Use existing wrangler installation when appropriate #235

Merged
merged 10 commits into from
May 15, 2024

Conversation

AdiRishi
Copy link
Contributor

@AdiRishi AdiRishi commented Feb 9, 2024

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.

@AdiRishi AdiRishi requested a review from a team as a code owner February 9, 2024 11:40
@AdiRishi AdiRishi changed the title (feat): Check and use existing wrangler installation (feat): Use existing wrangler installation when appropriate Feb 9, 2024
@Cherry
Copy link
Contributor

Cherry commented Feb 18, 2024

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 #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
Copy link
Contributor Author

AdiRishi commented Mar 2, 2024

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
Copy link
Contributor Author

AdiRishi commented Mar 5, 2024

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
Copy link
Contributor Author

AdiRishi commented Apr 4, 2024

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

@petebacondarwin
Copy link
Contributor

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
Copy link
Contributor Author

@petebacondarwin a gentle bump again on this PR 🙏

@petebacondarwin
Copy link
Contributor

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.

src/index.ts Outdated Show resolved Hide resolved
"license": "MIT",
"private": true,
"devDependencies": {
"wrangler": "^3.28.0"
Copy link
Contributor

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.

Suggested change
"wrangler": "^3.28.0"
"wrangler": "3.28.0"

Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor

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

Copy link
Contributor

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.

Copy link
Contributor Author

@AdiRishi AdiRishi May 6, 2024

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

@AdiRishi
Copy link
Contributor Author

AdiRishi commented May 2, 2024

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

@petebacondarwin
Copy link
Contributor

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
Copy link
Contributor Author

AdiRishi commented May 6, 2024

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
Copy link
Contributor Author

AdiRishi commented May 7, 2024

@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
Copy link
Contributor

@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
Copy link
Contributor Author

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

Copy link
Contributor

@petebacondarwin petebacondarwin left a 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
Copy link
Contributor

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...

Copy link
Contributor Author

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
Copy link
Contributor

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?

Copy link
Contributor Author

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.

@AdiRishi
Copy link
Contributor Author

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 :)

@petebacondarwin petebacondarwin merged commit 0545ad2 into cloudflare:main May 15, 2024
2 of 3 checks passed
@github-actions github-actions bot mentioned this pull request May 15, 2024
@AdiRishi AdiRishi deleted the arishi-issue-199 branch May 15, 2024 09:24
Maximo-Guk added a commit to Maximo-Guk/wrangler-action that referenced this pull request May 24, 2024
Maximo-Guk added a commit to Maximo-Guk/wrangler-action that referenced this pull request May 24, 2024
jahands added a commit that referenced this pull request May 24, 2024
Revert "(feat): Use existing wrangler installation when appropriate #235"
@github-actions github-actions bot mentioned this pull request May 24, 2024
Maximo-Guk added a commit to Maximo-Guk/wrangler-action that referenced this pull request May 24, 2024
AdiRishi added a commit to AdiRishi/wrangler-action that referenced this pull request Jun 9, 2024
Maximo-Guk added a commit that referenced this pull request Jun 16, 2024
Maximo-Guk added a commit that referenced this pull request Jun 20, 2024
Unreverts #235 and don't automatically install wrangler when checking if it present
@github-actions github-actions bot mentioned this pull request Jun 20, 2024
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.

Wrangler is reinstalled even when it's already available
4 participants