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

improve error message on invalid formatting of shell options #6680

Merged
merged 2 commits into from
Mar 4, 2025

Conversation

wihobbs
Copy link
Member

@wihobbs wihobbs commented Mar 3, 2025

Problem: When submitting a job through the command line, a shell option -o KEY[=VAL] is not required to have a value. If one is not provided, then attributes.system.shell.KEY is set to 1. If a user later provides a sub-key of this shell option, such as -o KEY.foo=bar, the error message returned by Python is not particularly helpful.

Wrap set_treedict() in a try/except where user input might provide an invalid key and raise a useful error message.

Fixes #6678

@wihobbs wihobbs force-pushed the issue-6678 branch 2 times, most recently from abc368f to a953f13 Compare March 3, 2025 23:10
Copy link
Contributor

@grondo grondo left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks!

wihobbs added 2 commits March 3, 2025 16:48
Problem: There is no test that ensures that `flux alloc -o foo
-o foo.bar=hi` returns an error message that is useful to an end
user.

Add such a test.
Problem: When submitting a job through the command line, a shell
option `-o KEY[=VAL]` is not required to have a value. If one is
not provided, then `attributes.system.shell.KEY` is set to 1. If a
user later provides a sub-key of this shell option, such as `-o
KEY.foo=bar`, the error message returned by Python is not
particularly helpful.

Wrap `set_treedict()` in a try/except where user input might
provide an invalid key and raise a useful error message.
Fixes flux-framework#6678
@grondo
Copy link
Contributor

grondo commented Mar 4, 2025

Just a minor point for next time. Not required, but we tend to try to keep the testsuite passing for each commit (sometimes helps with git bisect), so when adding tests, add them after fixing the bug or adding the feature on which the tests depend.

@wihobbs
Copy link
Member Author

wihobbs commented Mar 4, 2025

Oh, that makes sense. I was trying to show that I did test-driven-development ;)

@mergify mergify bot merged commit 50dcd44 into flux-framework:master Mar 4, 2025
34 of 35 checks passed
Copy link

codecov bot commented Mar 4, 2025

Codecov Report

Attention: Patch coverage is 75.00000% with 2 lines in your changes missing coverage. Please review.

Project coverage is 83.87%. Comparing base (621c7b8) to head (5acc90d).
Report is 3 commits behind head on master.

Files with missing lines Patch % Lines
src/bindings/python/flux/cli/base.py 50.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #6680      +/-   ##
==========================================
+ Coverage   83.85%   83.87%   +0.01%     
==========================================
  Files         534      534              
  Lines       88931    88937       +6     
==========================================
+ Hits        74577    74593      +16     
+ Misses      14354    14344      -10     
Files with missing lines Coverage Δ
src/bindings/python/flux/job/Jobspec.py 89.92% <100.00%> (+0.06%) ⬆️
src/bindings/python/flux/cli/base.py 95.51% <50.00%> (-0.26%) ⬇️

... and 10 files with indirect coverage changes

@grondo
Copy link
Contributor

grondo commented Mar 4, 2025

Oh, that makes sense. I was trying to show that I did test-driven-development ;)

Yeah, that makes sense too (and I usually write the tests first as well) :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve cli submission error message when -o key is used with -o key.subkey=val
2 participants