Fix tool argument handling (v0.69) #1812
Merged
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
portante
added
bug
Agent
tools
portante
changed the title
ndokos
previously approved these changes
Sep 22, 2020
Member
ndokos
left a comment
ndokos
left a comment
There was a problem hiding this comment.
LGTM - do we need to turn off black for this? The tests are failing because of it.
See PR distributed-system-analysis#1768, #d382a8da2b8bdac4e88fe0f6da24ec31fc6d1051.
portante
force-pushed
the
fix-postprocess-tools
branch
from
b243fa1 to
7f0d31d
Compare
Member
Author
I back-ported PR #1768 to this branch, which should fix that problem. The agent and server tests also failed, but they seem to be an ordering problem we have not locked down in the unit tests. |
portante
force-pushed
the
fix-postprocess-tools
branch
from
7f0d31d to
4419bd7
Compare
Member
Author
|
@ndokos, so I've had to add a couple more commits to address the test failures. Seems like we have a working environment now. |
portante
dismissed
ndokos’s stale review
Made some major, but subtle, changes.
This is a back-port of #bf382c23f2ddd1a1a97a6c035fbff2cc6f3c9313, from PR distributed-system-analysis#1733.
portante
force-pushed
the
fix-postprocess-tools
branch
2 times, most recently
from
0172419 to
6cb7649
Compare
This is a back-port of #47edfa80d9 (PR distributed-system-analysis#1734).
When `pbench-register-tool` accepts command line arguments to add as tool options, the order of those options received needs to be preserved. The code needs to use indexed arrays properly to preserve order. It was trying to use associative arrays as indexed. We also add a unit test for `pbench-start-tools`, using the `tcpdump` command with two arguments registered.
Fixes issue distributed-system-analysis#1751, where tools with multiple arguments only have the first argument passed to the tool command.
portante
force-pushed
the
fix-postprocess-tools
branch
from
6cb7649 to
beeff24
Compare
ndokos
approved these changes
Sep 23, 2020
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
Fixes issue #1751, where tools with multiple arguments only have the first argument passed to the tool command.
This PR is only for the
b0.69branch.