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

fix: Use wrangler secret put instead of deprecated secret:bulk command #328

Closed
wants to merge 1 commit into from

Conversation

jahands
Copy link
Contributor

@jahands jahands commented Nov 18, 2024

wrangler secret:bulk is deprecated and will be removed in a future version.

This improves logging in cases where a secret is failing to upload because an environment variable with the same name already exists (see: #240). Example:

Screenshot 2024-11-18 at 9 41 21 AM

With this change, we get this:
Screenshot 2024-11-18 at 9 35 43 AM

Testing:

Tested with a repo where the Worker already contains a secret of a different type:
https://github.com/jahands/wrangler-action-example/actions/runs/11895527967/job/33145312742

`wrangler secret:bulk` is deprecated and will be removed in a future
version. This also improves logging in cases where a secret is failing
to upload because an environment variable with the same name already
exists (see: #240).
@jahands jahands requested review from a team as code owners November 18, 2024 15:42
@@ -241,26 +241,6 @@ function getEnvVar(envVar: string) {
return value;
}

async function legacyUploadSecrets(
Copy link
Member

Choose a reason for hiding this comment

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

Should we keep the legacy upload behavior for people on older versions of wrangler?

Copy link
Member

Choose a reason for hiding this comment

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

Ohh, "wrangler", "secret:bulk" is being deprecated, and "wrangler", "secret", "put" is the current "legacy" way of doing things

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The legacy behavior is the current behavior :) The "new" behavior was the now-deprecated secret:bulk command.

All this PR does is replace the "non-legacy" behavior with the code from this function

@Maximo-Guk
Copy link
Member

We now have some baby tests in wranglerAction.test.ts #322 , so it's possible to write some tests for uploadSecrets() if you're so inclined!

Copy link
Member

@Maximo-Guk Maximo-Guk left a comment

Choose a reason for hiding this comment

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

Based off of @WalshyDev feedback, we should probably use cloudflare/workers-sdk#5988 wrangler secret bulk instead, if the user is on a newer version of wrangler where wrangler secret:bulk was deprecated

@Cherry
Copy link
Contributor

Cherry commented Nov 18, 2024

Yeah, this would be a pretty major behavioural change to revert to the old method of setting individual secrets. Using wrangler secret put for tens of secrets within a worker can result in errors from the API since they're run in parallel and sometimes don't get acknowledged quickly enough and then cause a mismatch, as well as that many individual new deploys of your worker, cluttering up the versions list. Not to mention drastically slow down the deploy time too.

@jahands jahands closed this Nov 18, 2024
@jahands
Copy link
Contributor Author

jahands commented Nov 18, 2024

Closing as this was not the right way to fix this. Looking into improving logging in bulk puts

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