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: support token with space like Bearer xxxxx #155

Merged
merged 7 commits into from
Sep 17, 2024

Conversation

davidB
Copy link
Contributor

@davidB davidB commented Sep 16, 2024

I guess, having the input token set with space like Bearer xxxx, is the cause of an error like

-- Running release-plz release --
error: unexpected argument '***' found

Usage: release-plz release [OPTIONS]

For more information, try '--help'.

(the *** is the output, hidden by github workflow because comes from a secret)

I guess, having token like `Bearer xxxx` is the cause of an error like

```sh
-- Running release-plz release --
error: unexpected argument '***' found

Usage: release-plz release [OPTIONS]

For more information, try '--help'.
```
@marcoieni
Copy link
Member

The test is failing. Did you test your branch and check that this fixed your issue?

@marcoieni
Copy link
Member

improved the error message here: release-plz/release-plz#1682

@davidB
Copy link
Contributor Author

davidB commented Sep 16, 2024

No, the workflow were not activated on my fork.
I'll fix it.

@davidB davidB marked this pull request as draft September 16, 2024 14:46
@marcoieni
Copy link
Member

ok, please also test your action directly against your project to check if this fix solves your issue.

"${CONFIG_PATH[@]}"\
"${ALT_REGISTRY[@]}"\
"${MANIFEST_PATH[@]}"\
"${BACKEND[@]}"\
Copy link
Member

Choose a reason for hiding this comment

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

is this trick used to work with whitespace? If yes, why are we using it also in the backend, which is one world only?
Same for ALT_REGISTRY probably

Choose a reason for hiding this comment

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

yes this trick is to keep the separation of argument clean as defined in the array. I also apply it BACKEND and ALT_REGISTRY for:

  • consistency (same pattern for every arguments fragment)
  • to guide if new argument should be added in the future (no question about which "form" to use)

@marcoieni
Copy link
Member

doesn't TOKEN="--token \"${{ inputs.token }}\"" work?
I.e. adding " around the token

@davidB
Copy link
Contributor Author

davidB commented Sep 17, 2024

doesn't TOKEN="--token \"${{ inputs.token }}\"" work? I.e. adding " around the token

No I tried it, same issue than with my first try with single quote, the double quote is included into the arguments when combined with $(...) or eval.

To test the various approach locally I used a small script to display every args.

#!/bin/bash 
for i in "$@"; do 
  echo "$i" 
done
bash-5.2$ V2="hello \"foo bar\""
bash-5.2$ ./my_cmd.sh ${V2}
hello
"foo
bar"

I remembered the tricks about array, but was not sure about the syntax, so quick search on SO: https://superuser.com/questions/360966/how-do-i-use-a-bash-variable-string-containing-quotes-in-a-command

@davidB davidB marked this pull request as ready for review September 17, 2024 07:56
@davidB
Copy link
Contributor Author

davidB commented Sep 17, 2024

FYI, the github workflow aren't triggered on my fork (I enable them, but no trigger).
But I just test it on one of my project with success. (that also confirms the fixed about HTTP 1.1 and the SSL issue I got on github-runner)

@marcoieni
Copy link
Member

Ok so this trick allows to use " inside an argument.
Why do you need "? Does your token contain this character?

@marcoieni
Copy link
Member

@davidB
Copy link
Contributor Author

davidB commented Sep 17, 2024

Ok so this trick allows to use " inside an argument.
Why do you need "? Does your token contain this character?

No, this trick allows to NOT have ". Using array allows to have args has expected:

# what we what (without variable)
bash-5.2$ ./my_cmd.sh "hello" "foo bar"
hello
foo bar

# if we just quote (single or double) inside the variable
bash-5.2$ V2="hello \"foo bar\""
bash-5.2$ ./my_cmd.sh ${V2}
hello
"foo
bar"
bash-5.2$ ./my_cmd.sh "${V2}"
hello "foo bar"

# solution with array
V1=("hello" "foo bar")
bash-5.2$ ./my_cmd.sh $V1
hello
bash-5.2$ ./my_cmd.sh $V1[@]
hello[@]
bash-5.2$ ./my_cmd.sh ${V1[@]}
hello
foo
bar
# hourra !!
bash-5.2$ ./my_cmd.sh "${V1[@]}"
hello
foo bar

# with empty array
bash-5.2$ V3=()
bash-5.2$ ./my_cmd.sh "${V3[@]}"
bash-5.2$ 

@davidB
Copy link
Contributor Author

davidB commented Sep 17, 2024

Also, "Bearer" is already set by release-plz:

https://github.com/MarcoIeni/release-plz/blob/e4cc526d9ee11e18c6319d72529fcb07ddebc352/crates/release_plz_core/src/git/github_client.rs#L40

I don't need "Bearer" for github token, but for crates registry's token

action.yml Show resolved Hide resolved
Copy link
Member

@marcoieni marcoieni left a comment

Choose a reason for hiding this comment

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

I'm this close to rewrite this bash script in typescript 😂
Thanks!

@marcoieni marcoieni merged commit 444ecf5 into release-plz:main Sep 17, 2024
1 check passed
@marcoieni
Copy link
Member

released 👍

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