add -S, --setattr and -c, --config-path options directly to flux start
#6452
Conversation
This file contains 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
|
Sounds like a great idea to me. |
grondo
changed the title
-S, --setattr and -c, --config-path options directly to flux start-S, --setattr and -c, --config-path options directly to flux start
|
Converted most of the testsuite to use |
Problem: The setup of common broker arguments is duplicated between `exec_broker()` and `client_create()`, which could result in inadvertent drift between the two methods for executing brokers. Add a new `add_args_common()` function and put the addition of common arguments there.
Problem: client_create() uses asprintf() to build a broker option when add_argzf() already exists for that purpose. Switch to add_argzf().
Problem: flux-start.c has a few lines that do not conform to modern project coding style. Break some long parameter lists into one per line, remove extraneous whitespace in a couple places, and shorten a few lines that were 80 characters or longer.
Problem: It would be useful to have a way to prepend a string to every argument added to the broker cmdline with add_args_list(), but this functionality does not exist. Add a prepend argument to add_args_list(). Update current callers to prepend an empty string.
Problem: Setting broker attributes from `flux start` is unnecessarily complicated by requiring the broker `--setattr` option to be passed via `-o, --broker-opts=`. But this indirection seems senseless because the only thing `flux start` does is start brokers. Add a `-S, --setattr=` option to `flux start` that just passes the same option down to the broker.
Problem: To specify the broker config path on the command line of `flux start`, the option must be passed via `-o, --broker-opts`, which is slightly inconvenient. Add a `-c, --config-path` option to `flux start` which just passes the same along to the broker.
Problem: add_argzf() truncates arguments at 1023 characters. Use vasnprintf() to dynamically allocate space for arguments to avoid the truncation.
Problem: The `-S, --setattr` and `-c, --config-path` options are not documented in flux-start(1). Document them.
Problem: Throughout the testsuite `-o-Sattr=val` and `-o,-cPATH` are used to pass broker attributes and config-path to Flux brokers, but this level of indirection is no longer necessary since flux-start supports `-S, --setattr` and `-c, --config-path` options. Update the testsuite to use `flux start` `-S, --setattr` and `-c, --config-path` options where it makes sense.
grondo
force-pushed
the
flux-start-newargs
branch
from
8e87c50 to
89c2016
Compare
|
Thanks, coverage is low just because there's a lot of error conditions we can't hit in test. I'll set MWP. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #6452 +/- ##
==========================================
+ Coverage 83.61% 83.63% +0.01%
==========================================
Files 523 523
Lines 87533 87538 +5
==========================================
+ Hits 73192 73210 +18
+ Misses 14341 14328 -13
|
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.
I kept getting annoyed that the common broker options for configuration path and setting attributes have to be passed through
flux start's-o, --broker-optsoption.flux startis used solely to start brokers, so why can't it just support these common options directly? So, submitted for your consideration is a PR that adds-S, --setattrand-c, --config-pathoptions directly toflux start. If used, these options are just passed along to the broker(s) being started.WIP because there's no tests or documentation updates in case there's some reason I'm not thinking of that this was a bad idea.