feat: Improve crashes, check apparatus defaults#75
Conversation
henry-wallace-phys
changed the title
MRiganSUSX
left a comment
MRiganSUSX
left a comment
There was a problem hiding this comment.
Thank @henry-wallace-phys.
Few comment below :)
| if apparatus is None: | ||
| raise ValueError( | ||
| "Apparatus must be specified with -a or APPARATUS env variable" | ||
| ) |
There was a problem hiding this comment.
Why not raise click.UsageError(...) ? That gives a clean "Error: ..." line and exits with code 2 (standard).
| raise ValueError( | ||
| "If any of base_url, ops_url, or config_file_name are not specified, all three must be resolved from the apparatus defaults." | ||
| ) |
There was a problem hiding this comment.
Same as before, wouldn't click.UsageError(...) be more appropriate ?
|
|
||
| # Matches lines like: export KEY="VALUE" or export KEY=${OTHER_VAR}/value | ||
| # Specifically captures the key and everything inside the quotes | ||
| export_pattern = re.compile(r'^\s*export\s+([A-Za-z0-9_]+)\s*=\s*"([^"]+)"') |
There was a problem hiding this comment.
Not necessarily problematic atm, but this way it very much depends on the setup scripts being always the same / right for this.
For example export BASE_URL='...' or export BASE_URL=https://... are skipped with this regex.
Suggested extended pattern:
export_pattern = re.compile(
r'^\s*export\s+([A-Za-z0-9_]+)\s*=\s*(?:"([^"]*)"'
r"|'([^']*)'|([^#\s]*))"
)
There was a problem hiding this comment.
I've implemented this as I agree this is sensible to have
| config_file_name = apparatus_defaults.get("config_file_name") | ||
| elif any(v is None for v in apparatus_vars): | ||
| raise ValueError( | ||
| "If any of base_url, ops_url, or config_file_name are not specified, all three must be resolved from the apparatus defaults." |
There was a problem hiding this comment.
Suggest changing this to
"Specify all of --base-url, --ops-url, and --config-file-name together, or omit all three to use apparatus defaults."
| :param apparatus: DAQ apparatus name (e.g., NP02, NP04) | ||
| :param config_directory: Path to configuration directory | ||
| :param output_directory: Directory to save run configs to | ||
| :param use_local: Use local filesystem instead of remote API | ||
| :param config_file_name: Config file to find in the ops repo | ||
| :param base_url: URL for the BASE repository | ||
| :param ops_url: URL for the operations repository | ||
| :param log_level: Log level (INFO, WARNING, DEBUG) |
There was a problem hiding this comment.
I know you removed this because it was causing some --help problems or similar, but... isn't there a solution where we can have both proper --help and a docstring ? Some of the parameters' env-var fallbacks and mutual-exclusion rules were only documented there.
There was a problem hiding this comment.
[2 mins of reading documentation later....] This is now re-implemented and doesn't print with --help
3 participants
Description
Testing: Start runconf-shifter-ui with either
-a np02or-a np04. Also test start-up after runningrunconf_np02_env_setup.sh. Additionally test breaking path with either no-a(and no setup script) and-a footo check for breaking paths (no apparatus or non-existent apparatus)Type of change
Testing checklist
dbt-build --unittest)pytest -s minimal_system_quick_test.py)daqsystemtest_integtest_bundle.sh)python -m pytest)pre-commit run --all-files)Comments here on the testing
Further checks
dbt-build --lint, and/or see https://dune-daq-sw.readthedocs.io/en/latest/packages/styleguide/)(Indicate issue here: # (issue))