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] Reapply shell script parameter passthrough fix #22867 reverted in #22921 #22923

Merged
merged 1 commit into from
Oct 9, 2024

Conversation

lhotari
Copy link
Member

@lhotari lhotari commented Jun 17, 2024

This reverts commit fa74538.

Motivation

#22921 reverted the change in #22867 to use correct way of passing parameters in shell scripts.
The correct syntax is "$@".

In #22921, the argumentation was that a command bin/pulsar zookeeper-shell --run-once "ls /ledgers" no longer works.
This type of command has always been invalid for 2 reasons:

  • bin/pulsar zookeeper-shell has never supported --run-once parameter.
    • there's a Python based zk-shell which supports this syntax. bin/pulsar zookeeper-shell doesn't use zk-shell under the covers.
  • bin/pulsar zookeeper-shell has never supported quoting the arguments to the command. This happened to work because of invalid parameter passing which is fixed by [fix][cli] Fix the shell script parameter passthrough syntax #22867
    • The Python based zk-shell requires the --run-once "ls /path" syntax. bin/pulsar zookeeper-shell doesn't use zk-shell under the covers.

Modifications

Documentation

  • doc
  • doc-required
  • doc-not-needed
  • doc-complete

@lhotari lhotari added this to the 3.4.0 milestone Jun 17, 2024
@lhotari lhotari requested a review from coderzc June 17, 2024 10:43
@lhotari lhotari self-assigned this Jun 17, 2024
@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Jun 17, 2024
@lhotari lhotari changed the title [fix] Revert the invalid revert of #22867 in #22921 [fix] Reapply shell script parameter passthrough fix #22867 reverted in #22921 Jun 25, 2024
@lhotari
Copy link
Member Author

lhotari commented Jun 25, 2024

I hope we get the scripts fixed, at least for master branch. It's clearly a problem to use $@ without double quotes to pass parameters on to another command. That's why I think that this change should be made "$@".

@lhotari lhotari added the release/blocker Indicate the PR or issue that should block the release until it gets resolved label Oct 9, 2024
@Technoboy-
Copy link
Contributor

Technoboy- commented Oct 9, 2024

$@
When you use $@ without quotes, it expands to each positional parameter as a separate word. This means that if any of the arguments contain spaces, they will be split into multiple words. For example, if a script is run with the arguments one two and three, $@ would treat them as three separate arguments: one, two, and three.
"$@"
When you use "$@" with double quotes, it expands to each positional parameter preserved as a single word, regardless of spaces. This means that each argument is treated exactly as it was passed, including spaces. For example, if a script is run with the arguments “one two“ and three, "$@" would treat them as two arguments: ”one two“ and three.

@lhotari lhotari closed this Oct 9, 2024
@lhotari lhotari reopened this Oct 9, 2024
@lhotari lhotari merged commit b051dcd into apache:master Oct 9, 2024
108 of 115 checks passed
@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 74.32%. Comparing base (bbc6224) to head (28a69ee).
Report is 653 commits behind head on master.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #22923      +/-   ##
============================================
+ Coverage     73.57%   74.32%   +0.75%     
- Complexity    32624    34350    +1726     
============================================
  Files          1877     1949      +72     
  Lines        139502   146867    +7365     
  Branches      15299    16168     +869     
============================================
+ Hits         102638   109160    +6522     
- Misses        28908    29284     +376     
- Partials       7956     8423     +467     
Flag Coverage Δ
inttests 27.54% <ø> (+2.95%) ⬆️
systests 24.44% <ø> (+0.12%) ⬆️
unittests 73.67% <ø> (+0.82%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

see 621 files with indirect coverage changes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc-not-needed Your PR changes do not impact docs ready-to-test release/blocker Indicate the PR or issue that should block the release until it gets resolved
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants