HBASE-26543 correct parsing of shell args with GetoptLong #4000
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
🎊 +1 overall
This message was automatically generated. |
madrob
reviewed
Jan 5, 2022
| ) | ||
| opts.ordering = GetoptLong::REQUIRE_ORDER |
There was a problem hiding this comment.
Is this going to break back compat somewhere?
There was a problem hiding this comment.
no, this ordering option should bring us closer to before HBASE-24772 was introduced. I reasonably believe it is compatible with our parsing prior to that change.
There was a problem hiding this comment.
for those interested in more details here are the docs for Ruby GetoptLong's ordering:
https://ruby-doc.org/stdlib-3.0.3/libdoc/getoptlong/rdoc/GetoptLong.html#method-i-ordering-3D
specifically, this patch expressly has us use REQUIRE_ORDER:
REQUIRE_ORDER :
Options are required to occur before non-options.
Processing of options ends as soon as a word is encountered that has not been preceded by an appropriate option flag.
I believe this matches the behavior of our manual cli argument parsing prior to HBASE-24772.
Note that by default GetoptLong picks an ordering based on an environment variable we don't set. The default in that case is PERMUTE :
PERMUTE :
Options can occur anywhere in the command line parsed. This is the default behavior.
Every sequence of words which can be interpreted as an option (with or without argument) is treated as an option; non-option words are skipped.
This ordering option is why after HBASE-24772, and prior to this patch, passing in IRB's "ignore .irbrc" flag ( -f) would cause our shell to crash with an "I don't know what that option is supposed to mean".
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
LGTM |
madrob
approved these changes
Jan 7, 2022
busbey
added a commit
that referenced
this pull request
Jan 8, 2022
Signed-off-by: Mike Drob <mdrob@apache.org>
busbey
added a commit
that referenced
this pull request
Jan 8, 2022
Signed-off-by: Mike Drob <mdrob@apache.org> (cherry picked from commit dda337f)
busbey
added a commit
that referenced
this pull request
Jan 8, 2022
Signed-off-by: Mike Drob <mdrob@apache.org> (cherry picked from commit dda337f)
szucsvillo
pushed a commit
to szucsvillo/hbase
that referenced
this pull request
Sep 9, 2024
…etoptLong (apache#4000) Signed-off-by: Mike Drob <mdrob@apache.org> (cherry picked from commit dda337f) (cherry picked from commit 9048507) Change-Id: I48f9c38241aa5e546d77f87e2ce3645cba9ce507
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Since Ruby's getoptlong library can't handle config definitions of the form
-Dkey=value, we preprocess all args to pull out config definitions.After that preprocessing, the remaining changes correct our use of the getoptlong library so we have the same cli handling as prior to using the getoptlong library.
Local testing, see HBASE-26543 for previous behavior: