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

Clarify what tool-specific options are #3444

Merged
merged 1 commit into from
Jun 2, 2023

Conversation

ndokos
Copy link
Member

@ndokos ndokos commented May 31, 2023

Fixes #3443

Modify the help message of pbench-register-tool to warn against using generic options for a tool when it is registered.

Modify the help message of base-tool (and everybody who is symlinked to it) to clarify which are generic options and which are tool-specific options.

PBENCH-1173

@ndokos ndokos added bug Agent Documentation tools Of and related to the operation and behavior of various tools (iostat, sar, etc.) labels May 31, 2023
@ndokos ndokos added this to the v0.72 milestone May 31, 2023
@ndokos ndokos requested review from webbnh and riya-17 May 31, 2023 18:11
@ndokos ndokos self-assigned this May 31, 2023
@ndokos ndokos marked this pull request as draft May 31, 2023 18:55
webbnh
webbnh previously approved these changes May 31, 2023
Copy link
Member

@webbnh webbnh left a comment

Choose a reason for hiding this comment

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

Looks good, although I would go with something which stands in opposition to "registration" (like "operational") instead of "generic".

agent/tool-scripts/base-tool Outdated Show resolved Hide resolved
agent/tool-scripts/base-tool Outdated Show resolved Hide resolved
@ndokos ndokos marked this pull request as ready for review May 31, 2023 20:46
@ndokos ndokos force-pushed the tool-script-help branch from 7d988ff to 066a790 Compare May 31, 2023 21:02
@ndokos ndokos requested review from dbutenhof and webbnh May 31, 2023 21:03
@ndokos
Copy link
Member Author

ndokos commented May 31, 2023

OK - once more with feeling!

webbnh
webbnh previously approved these changes May 31, 2023
Copy link
Member

@webbnh webbnh left a comment

Choose a reason for hiding this comment

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

It just keeps getting better (but, there are always opportunities for improvements...).

agent/tool-scripts/tests/blktrace/gold/stderr Outdated Show resolved Hide resolved
agent/tool-scripts/tests/lockstat/gold/stderr Outdated Show resolved Hide resolved
agent/tool-scripts/base-tool Outdated Show resolved Hide resolved
dbutenhof
dbutenhof previously approved these changes Jun 1, 2023
Copy link
Member

@dbutenhof dbutenhof left a comment

Choose a reason for hiding this comment

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

While I'm tempted to "request changes" for the indentation glitch, and Webb's suggestion about the tool options would help a lot with readability in those cases, I guess neither are critical so I'll approve with only slight misgivings for the sake of keeping 0.72 moving 😁 ...

agent/tool-scripts/base-tool Outdated Show resolved Hide resolved
agent/tool-scripts/base-tool Outdated Show resolved Hide resolved
@ndokos ndokos dismissed stale reviews from dbutenhof and webbnh via b8079de June 1, 2023 22:09
@ndokos ndokos force-pushed the tool-script-help branch from 066a790 to b8079de Compare June 1, 2023 22:09
@ndokos
Copy link
Member Author

ndokos commented Jun 1, 2023

Rebased and squashed.

@ndokos ndokos requested review from webbnh and dbutenhof June 1, 2023 22:10
Copy link
Member

@webbnh webbnh left a comment

Choose a reason for hiding this comment

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

It looks like the strace handling needs tweaking (and there's one other tweak to make while you're at it).

agent/tool-scripts/base-tool Outdated Show resolved Hide resolved
agent/tool-scripts/tests/lockstat/gold/stderr Outdated Show resolved Hide resolved
agent/tool-scripts/tests/strace/gold/stderr Outdated Show resolved Hide resolved
Fixes distributed-system-analysis#3443

Modify the help message of pbench-register-tool to warn against using
operational options (i.e. options used internally by pbench) for a
tool when it is registered.

Modify the help message of base-tool (and everybody who is symlinked
to it) to clarify which are operational options and which are
tool-specific options: operational options should not be specified
when registering a tool: they are used by pbench internally; only
tool-specific options are specifed when registering the tool (on the
RHS of `--` in the invocation of `pbench-register-tool`).

PBENCH-1173
@ndokos ndokos force-pushed the tool-script-help branch from b8079de to 2110ad8 Compare June 2, 2023 02:28
@ndokos ndokos requested a review from webbnh June 2, 2023 02:30
Copy link
Member

@webbnh webbnh left a comment

Choose a reason for hiding this comment

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

🎉

@ndokos ndokos merged commit c560bdf into distributed-system-analysis:b0.72 Jun 2, 2023
@ndokos ndokos deleted the tool-script-help branch June 2, 2023 16:24
ndokos added a commit to ndokos/pbench that referenced this pull request Jun 13, 2023
)

Fixes distributed-system-analysis#3443

Modify the help message of pbench-register-tool to warn against using
operational options (i.e. options used internally by pbench) for a
tool when it is registered.

Modify the help message of base-tool (and everybody who is symlinked
to it) to clarify which are operational options and which are
tool-specific options: operational options should not be specified
when registering a tool: they are used by pbench internally; only
tool-specific options are specifed when registering the tool (on the
RHS of `--` in the invocation of `pbench-register-tool`).

PBENCH-1173
ndokos added a commit to ndokos/pbench that referenced this pull request Jul 3, 2023
)

Fixes distributed-system-analysis#3443

Modify the help message of base-tool (and everybody who is symlinked
to it) to clarify the tool-specific options that may be specified when
registering a tool (on the RHS of `--` in the invocation of
`pbench-register-tool`).

PBENCH-1173
ndokos added a commit to ndokos/pbench that referenced this pull request Jul 3, 2023
)

Fixes distributed-system-analysis#3443

Modify the help message of base-tool (and everybody who is symlinked
to it) to clarify the tool-specific options that may be specified when
registering a tool (on the RHS of `--` in the invocation of
`pbench-register-tool`).

PBENCH-1173
ndokos added a commit that referenced this pull request Jul 3, 2023
Fixes #3443

Modify the help message of base-tool (and everybody who is symlinked
to it) to clarify the tool-specific options that may be specified when
registering a tool (on the RHS of `--` in the invocation of
`pbench-register-tool`).

PBENCH-1173
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Agent bug Documentation tools Of and related to the operation and behavior of various tools (iostat, sar, etc.)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants