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 (Forward port of #3444 from b0.72 to main) #3462

Merged
merged 1 commit into from
Jul 3, 2023

Conversation

ndokos
Copy link
Member

@ndokos ndokos commented Jun 13, 2023

Fixes #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 self-assigned this Jun 13, 2023
@ndokos ndokos added this to the v0.73 milestone Jun 13, 2023
dbutenhof
dbutenhof previously approved these changes Jun 13, 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.

OK; but I wonder if it would be clearer (and simpler) to just "lie" and not expose those internal "operational" options in the tool help in the first place to avoid confusion? (hidden=True in Click; or just omit from usage in bash...)

webbnh
webbnh previously approved these changes Jun 16, 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.

I'm feeling favorably disposed toward Dave's suggestion: since the "operational" options are not intended for end-users, perhaps they should be omitted or hidden from the tools' help output (at least by default). (There are several ways to do this properly, but probably none of them are worth the effort...so we probably should just comment them out.)

Short of that, we should probably pick a word other than "should", which is a little strong. Perhaps "may" would work, or we could add some weasel words like "as appropriate". And, there are at least two places where we should disentangle the "registration options" from the "operational" ones.

But, I'm not going to insist on holding the merge for these....

agent/tool-scripts/tests/blktrace/gold/stderr Outdated Show resolved Hide resolved
agent/tool-scripts/tests/cpuacct/gold/stderr Outdated Show resolved Hide resolved
@ndokos ndokos closed this Jun 27, 2023
@ndokos ndokos deleted the fwd-port-3444 branch June 27, 2023 15:57
@ndokos ndokos restored the fwd-port-3444 branch June 28, 2023 11:51
@ndokos ndokos reopened this Jun 28, 2023
@ndokos ndokos dismissed stale reviews from dbutenhof and webbnh via bc37e67 July 3, 2023 07:28
@ndokos ndokos requested review from webbnh and dbutenhof July 3, 2023 07:33
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.

👍

)

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 ndokos merged commit d6b2ed9 into distributed-system-analysis:main Jul 3, 2023
@ndokos ndokos deleted the fwd-port-3444 branch July 3, 2023 21:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

[BUG] Clarify what tool-specific options are
3 participants