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.

Sign up for GitHub

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

Jump to bottom

Update default wrangler version to 3.x #238

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Update default wrangler version to 3.x #238

wants to merge 5 commits into from

Conversation

Eusebiotrigo
Copy link

No description provided.

@Eusebiotrigo Eusebiotrigo requested a review from a team as a code owner February 29, 2024 14:52
@mrbbot
Copy link

mrbbot commented Mar 25, 2024

Hey! 👋 Apologies for not reviewing this sooner. This wrangler version is now out of date again... 😅 We should set up some automation to bump this automatically. 👍 Would you be able to bump this to 3.37.0 and add a changeset similar to 473d9cb?

@Eusebiotrigo Eusebiotrigo changed the title Update default wrangler version to 3.30.0 Update default wrangler version to 3.37.0 Mar 25, 2024
@Eusebiotrigo
Copy link
Author

@mrbbot all done, but I'm afraid there is a new version already...

petebacondarwin
petebacondarwin approved these changes Apr 15, 2024
View reviewed changes
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.

Let's just land this. Going forward we should try to implement dependabot to do this automatically ... @nprogers

@1000hz
Copy link
Contributor

1000hz commented Apr 15, 2024

IMO, we should rework the action to use latest as a last resort default (rather than some hardcoded version number that'll quickly fall out of date), with the following precedence order for determining the wrangler version used by the action:

  1. If the wranglerVersion input was specified, check for an existing wrangler installation in the PATH that satisfies that version. If so, use it. Else, install the specified version and use that instead.
  2. If no wranglerVersion was specified, check for an existing wrangler installation in the PATH (most likely declared in package.json and installed by a previous "install packages" step of the workflow). If so, use it. Else, install wrangler@latest and use that.

@RamIdeas
Copy link
Contributor

IMO, we should rework the action to use latest as a last resort default (rather than some hardcoded version number that'll quickly fall out of date), with the following precedence order for determining the wrangler version used by the action:

  1. If the wranglerVersion input was specified, check for an existing wrangler installation in the PATH that satisfies that version. If so, use it. Else, install the specified version and use that instead.
  2. If no wranglerVersion was specified, check for an existing wrangler installation in the PATH (most likely declared in package.json and installed by a previous "install packages" step of the workflow). If so, use it. Else, install wrangler@latest and use that.

Makes sense LGTM 👍

@petebacondarwin
Copy link
Contributor

In that case I think we should leave @nevikashah to take a call on this and hand it over to the new team that owns this repo now.

@Eusebiotrigo
Copy link
Author

Oh! It is 3.52.0 now...

@Eusebiotrigo Eusebiotrigo changed the title Update default wrangler version to 3.37.0 Update default wrangler version to 3.52.0 Apr 29, 2024
@petebacondarwin
Copy link
Contributor

OK - so thinking about this a bit more.

Rather than constantly bumping this, I propose that we land #235 and then set change DEFAULT_WRANGLER_VERSION to latest.

Then the user has two ways to lock this down, either by installing wrangler locally or specifying a version in the wrangler-action config. Otherwise they just get latest.

How do people feel about that?

@Cherry
Copy link
Contributor

Cherry commented May 2, 2024

OK - so thinking about this a bit more.

Rather than constantly bumping this, I propose that we land #235 and then set change DEFAULT_WRANGLER_VERSION to latest.

Then the user has two ways to lock this down, either by installing wrangler locally or specifying a version in the wrangler-action config. Otherwise they just get latest.

How do people feel about that?

latest sounds dangerous and like a breaking change waiting to happen. I think v3 or 3.x.x would be fine though.

@petebacondarwin
Copy link
Contributor

latest sounds dangerous and like a breaking change waiting to happen. I think v3 or 3.x.x would be fine though.

Yeah I actually meant to say 3.x when I said latest 😄

@Taewan-P Taewan-P mentioned this pull request May 5, 2024
Closed
@kesar kesar mentioned this pull request May 14, 2024
Open
Eusebiotrigo and others added 5 commits May 15, 2024 14:04
@Eusebiotrigo @petebacondarwin
Update default wrangler version to 3.30.0
1d28a05
@Eusebiotrigo @petebacondarwin
Update default wrangler version to 3.37.0
a99a395
@Eusebiotrigo @petebacondarwin
Adding changeset
b959cd2
@Eusebiotrigo @petebacondarwin
Update default wrangler version to 3.52.0
888649c
@petebacondarwin
feature: Set DEFAULT_WRANGLER_VERSION to 3.x
9580f80 
This will ensure that wrangler-action will use the latest compatible version of Wrangler if not specified otherwise.

There are two ways to lock down the version of Wrangler for this action:

- Specify the required version in the action's parameters when implementing it in your Github Workflows.
- Add a dependency to a specific version in your package.json of the project being deployed via this action.
@petebacondarwin petebacondarwin changed the title Update default wrangler version to 3.52.0 Update default wrangler version to 3.x May 15, 2024
petebacondarwin
petebacondarwin reviewed May 15, 2024
View reviewed changes
src/index.ts
@@ -16,7 +16,7 @@ import { exec, execShell } from "./exec";
import { checkWorkingDirectory, semverCompare } from "./utils";
import { getPackageManager } from "./packageManagers";

const DEFAULT_WRANGLER_VERSION = "3.13.2";
const DEFAULT_WRANGLER_VERSION = "3.x";
Copy link
Contributor

Choose a reason for hiding this comment

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

Urgh! We can't set this to a range because we need to compare it with other semantic versions.

We could just go with latest here...?

Copy link
Contributor

Choose a reason for hiding this comment

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

latest would potentially introduce a breaking chance once 4.x is out though, wouldn't it, if people are just relying on the defaults?

Perhaps looking up the versions on npm and pulling the latest 3.x from there would be best as a first step? If there was a v3 tag, it could be as simple as (pseudo-example) curl https://registry.npmjs.org/wrangler | jq '.["dist-tags"].v3'.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah I briefly played around with looking up the version but it is a bit messy especially if there is no dist-tag, since we would have to iterate over all the versions and find one that best matches the version range.

@nprogers
Copy link

@petebacondarwin @1000hz to confirm, there is a way around this? a user can specify the version in their package.json? just trying to understand the urgency here

@1000hz
Copy link
Contributor

1000hz commented May 21, 2024 •
edited

@petebacondarwin @1000hz to confirm, there is a way around this? a user can specify the version in their package.json? just trying to understand the urgency here

Yes, I believe now that #235 has been merged this is much less urgent. We should still eventually figure out what a good fallback default value ought to be though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@petebacondarwin petebacondarwin petebacondarwin approved these changes

@Cherry Cherry Cherry left review comments

@kesar kesar kesar approved these changes

Assignees

@mrbbot mrbbot

Labels
awaiting Cloudflare response
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@Eusebiotrigo @mrbbot @1000hz @RamIdeas @petebacondarwin @Cherry @nprogers @kesar