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 parsing of secrets containing '=' character #201

Conversation

mathieubergeron
Copy link
Contributor

Fixed the parsing of secrets, which were not working when the secret contains = character. I also added a test for buildx.getSecret(..).

Looking at other PRs, it seems that my dist/index.js file contains way more changes than it should. Am I doing something wrong? I followed the CONTRIBUTING.md procedure.

Should I create an Issue for that PR?

Signed-off-by: Mathieu Bergeron <mathieu.bergeron@nuecho.com>
@codecov-io
Copy link

codecov-io commented Oct 23, 2020

Codecov Report

Merging #201 into master will increase coverage by 0.38%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #201      +/-   ##
==========================================
+ Coverage   73.52%   73.91%   +0.38%     
==========================================
  Files           4        4              
  Lines         136      138       +2     
  Branches       24       24              
==========================================
+ Hits          100      102       +2     
  Misses         21       21              
  Partials       15       15              
Impacted Files Coverage Δ
src/buildx.ts 77.08% <100.00%> (+0.99%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b3b0ca3...c8e09bf. Read the comment docs.

@mathieubergeron mathieubergeron force-pushed the fix-parse-secret-containing-equal-character branch from 3738a55 to fc7e9a2 Compare October 23, 2020 17:31
Signed-off-by: Mathieu Bergeron <mathieu.bergeron@nuecho.com>
@crazy-max
Copy link
Member

@mathieubergeron

Fixed the parsing of secrets, which were not working when the secret contains = character. I also added a test for buildx.getSecret(..).

LGTM thanks.

Looking at other PRs, it seems that my dist/index.js file contains way more changes than it should. Am I doing something wrong? I followed the CONTRIBUTING.md procedure.

What version of Node/Yarn do you use?

@mathieubergeron
Copy link
Contributor Author

What version of Node/Yarn do you use?

$ node --version
v10.15.0
$ yarn --version
1.12.3

Thanks!

@crazy-max
Copy link
Member

crazy-max commented Oct 23, 2020

@mathieubergeron

node --version
v10.15.0

That might be the issue, the GitHub Runner and this action uses Node 12. Can you do a pre-checkin with Node 12?

Are you on MacOS btw?

@mathieubergeron
Copy link
Contributor Author

Are you on MacOS btw?

No, I'm on Ubuntu

the GitHub Runner and this action uses Node 12.

Thanks for the pointer, I installed v12. I also updated yarn to 1.22.5.

@crazy-max I ran yarn run pre-checkin on master, and I'm still getting a different dist/index.js file. 🤔

$ node --version
v12.18.3

$ yarn run pre-checkin
yarn run v1.22.5
$ yarn run format && yarn run build
$ prettier --write **/*.ts
__tests__/buildx.test.ts 433ms
__tests__/context.test.ts 300ms
src/buildx.ts 236ms
src/context.ts 247ms
src/exec.ts 45ms
src/main.ts 134ms
src/state-helper.ts 14ms
$ tsc && ncc build
ncc: Version 0.23.0
ncc: Compiling file index.js
463kB  dist/index.js
463kB  [3299ms] - ncc 0.23.0
Done in 19.55s.

Signed-off-by: Mathieu Bergeron <mathieu.bergeron@nuecho.com>
@mathieubergeron mathieubergeron force-pushed the fix-parse-secret-containing-equal-character branch from 2fe061c to 8616d52 Compare October 23, 2020 19:02
@crazy-max
Copy link
Member

@mathieubergeron

I ran yarn run pre-checkin on master, and I'm still getting a different dist/index.js file.

Maybe linked to vercel/ncc#485. Can you checkout the master branch and run a pre-checkin from there just to be sure? Thanks.

@mathieubergeron
Copy link
Contributor Author

@crazy-max

Can you checkout the master branch and run a pre-checkin from there just to be sure? Thanks.

That's what I did in my previous response :-) And the generated dist/index.js is indeed different even on the master branch.

@crazy-max
Copy link
Member

crazy-max commented Oct 23, 2020

@mathieubergeron Ok I was on WSL2 and on a fresh Ubuntu I've got the same result as you so it LGTM :) I will create another PR for reproducibility and keep you in touch here.

@mathieubergeron
Copy link
Contributor Author

mathieubergeron commented Oct 23, 2020

@crazy-max

Ok I was on WSL2 and on a fresh Ubuntu I've got the same result as you so it LGTM :) I will create another PR for reproducibility and keep you in touch here.

Great! Thanks!

Copy link
Member

@crazy-max crazy-max left a comment

Choose a reason for hiding this comment

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

@mathieubergeron Can you merge changes from master?

…containing-equal-character

# Conflicts:
#	__tests__/buildx.test.ts
Copy link
Member

@crazy-max crazy-max left a comment

Choose a reason for hiding this comment

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

LGTM thanks!

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