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: secret input trailing spaces #1044

Merged
merged 1 commit into from
May 18, 2022
Merged

fix: secret input trailing spaces #1044

merged 1 commit into from
May 18, 2022

Conversation

JacobMGEvans
Copy link
Contributor

@JacobMGEvans JacobMGEvans commented May 17, 2022

Added trimTrailingWhitespace() to secretValue being sent in PUT request
Matching Wrangler legacy behavior with handling inputs

resolves #993

@changeset-bot
Copy link

changeset-bot bot commented May 17, 2022

🦋 Changeset detected

Latest commit: a05478f

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
wrangler Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@github-actions
Copy link
Contributor

github-actions bot commented May 17, 2022

A wrangler prerelease is available for testing. You can install this latest build in your project with:

npm install --save-dev https://prerelease-registry.developers.workers.dev/runs/2346143277/npm-package-wrangler-1044

You can reference the automatically updated head of this PR with:

npm install --save-dev https://prerelease-registry.developers.workers.dev/prs/1044/npm-package-wrangler-1044

Or you can use npx with this latest build directly:

npx https://prerelease-registry.developers.workers.dev/runs/2346143277/npm-package-wrangler-1044 dev path/to/script.js

@JacobMGEvans JacobMGEvans added the bug Something that isn't working label May 17, 2022
@petebacondarwin
Copy link
Contributor

Trimming the secret value would be a breaking change. Currently I can run

$ npx wrangler secret put foo

Then type in the characters " abc ". And these spaces are still available in my Worker.

@petebacondarwin
Copy link
Contributor

I think instead we only want to remove a trailing \n and only when reading from stdin.

@JacobMGEvans JacobMGEvans force-pushed the trim-secret-input branch 2 times, most recently from 5b79371 to 60d3942 Compare May 17, 2022 22:10
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.

Thanks @JacobMGEvans - just a couple of minor requests before we merge.

Comment on lines 5 to 6
fix: added `trimTrailingWhitespace()` to secretValue being sent in PUT request
Matching Wrangler legacy behavior with handling inputs
Copy link
Contributor

Choose a reason for hiding this comment

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

We should try to write these changeset descriptions from the point of view of the changelog reader. So I would describe the external effect rather than the implementation change in the heading. E.g.

fix: trim trailing whitespace from the secrets before uploading

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. Thanks!

packages/wrangler/src/__tests__/secret.test.ts Outdated Show resolved Hide resolved
…T request

Matching Wrangler legacy behavior with handling inputs

resolves #993
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.

LGTM!

@petebacondarwin petebacondarwin merged commit 7a191a2 into main May 18, 2022
@petebacondarwin petebacondarwin deleted the trim-secret-input branch May 18, 2022 18:40
@github-actions github-actions bot mentioned this pull request May 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something that isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

🐛 BUG: the value read from stdin should be trimmed
2 participants