Skip to content

Conversation

sdelliot
Copy link
Collaborator

This PR adds the ability for users to take in a properly formatted JSON string and set the FIREWHEEL configuration.

This JSON string will set/replace a subset of the configuration and any keys or sub-keys (or sub-sub-keys) not present will not be impacted or overwritten.
The string must include the top-level config key as well as any sub-keys needing modifications.

For example, to change the value for the config key logging.level from DEBUG to INFO, here is how that would not look:

Before Config

logging:
    cli_log: cli.log
    discovery_log: discovery.log
    firewheel_log: firewheel.log
    level: DEBUG
    minimega_log: minimega.log
    root_dir: /scratch/firewheel
    vmr_log_dir: vm_resource_logs
firewheel config set -j '{"logging":{"level":"INFO"}}'

After Config

logging:
    cli_log: cli.log
    discovery_log: discovery.log
    firewheel_log: firewheel.log
    level: INFO
    minimega_log: minimega.log
    root_dir: /scratch/firewheel
    vmr_log_dir: vm_resource_logs

@github-actions github-actions bot added documentation Improvements or additions to documentation feature New feature or request labels Sep 10, 2025
@mitchnegus
Copy link
Member

mitchnegus commented Sep 11, 2025

I like that it will not require a user to have to pass the whole block to set one element. Do we need to consider whether there is ever a need to remove key-value pairs from the configuration? (In which case, this removes the method of just omitting it from the JSON as an option to accomplish that).

Maybe we just say that a FIREWHEEL config should have all the settings defined, and so we don't support removing a field. Not sure that's best, because I don't know how a user would say "go back to the default" (other than checking the docs and manually setting it to that value, which seems like an unfriendly thing to do).

If we do establish that expectation, it's probably worth being aware that the config-template.yml file seems to include all the options by default except system.firewheel_root_dir. (Doesn't break anything here, but it would be a violation of our new "rule" that every config file contains every option. Also interestingly we don't seem to use system.firewheel_root_dir anywhere in this repo.)

argparse.ArgumentTypeError: If the input string is not a valid JSON.
"""
try:
return json.loads(json_string.replace("'", ""))
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure what the rationale behind this replacement is (I assume somewhere the command line passes unwanted single-quotes).

Since it's applied to the entire string, do we need to make a note somewhere that single quotes are not permitted in any of the config values? I don't know why a user would ever add a single quote to any of the values we offer right now, but if they did (or we add a config value where they might), I think this will silently just remove it...

@mitchnegus
Copy link
Member

Looks good to me other than the comments above.

To be "that guy", do we need/want a test or two?

@mitchnegus mitchnegus closed this Sep 11, 2025
@mitchnegus mitchnegus reopened this Sep 11, 2025
@gregjacobus
Copy link
Collaborator

I think it still makes sense to include this feature, but another option would be to use yq to modify the config file directly. This is probably what we should have done in #162 instead of that horrible sed command in hindsight. Downside is it requires installing something extra

@sdelliot sdelliot marked this pull request as draft September 11, 2025 12:00
@sdelliot
Copy link
Collaborator Author

I think it still makes sense to include this feature, but another option would be to use yq to modify the config file directly. This is probably what we should have done in #162 instead of that horrible sed command in hindsight. Downside is it requires installing something extra

While I don't like adding another dependency, this would "only" be the case for the CI, and won't impact end users. Additionally, this would be a wholly optional thing for users. Another benefit to this approach is less code maintenance that we have to write, which is always a positive. On the other hand, yq looks challenging to understand/use. However, I suspect that we could get away with a brief example/reference in the config set documentation to point users in the right way.

I'm going to play with yq to see how easy it is to replace that sed command and leave this in draft until then.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants