-
Notifications
You must be signed in to change notification settings - Fork 90
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
Fix bugs in cylc broadcast
#5933
Conversation
Everything after `#` in a setting was getting stripped out
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Tried it before and after - looks like it fixes both bugs.
- Read the code. Made comments, but neither is critical.
cylc/flow/parsec/validate.py
Outdated
if isinstance(value, str) and '#' in value: | ||
value = f'"{value}"' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't work if the string is already quoted e.g:
$ cylc broadcast one -n root -s 'script="sleep 30 # ja!"'
ERROR: [{'error': {'message': '(type=string) script = ""sleep 30 # ja!""', 'traceback': ['graphql.error.located_error.GraphQLLocatedError: (type=string) script = ""sleep 30 # ja!""\n']}}]
Maybe use shlex.quote
which has logic to avoid surplus quoting.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shlex.quote
messes with the quotes inside which we don't want. I've come up with a less hacky approach
val = ParsecValidator._unquote(keys, value) or value | ||
return dedent(val).strip() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should the dedent/strip happen before the _unquote?
If not, wouldn't 'foo'
end up as " 'foo' "
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This stripping is for the stuff inside the quotes, and this is the same as it was before this PR. I've tested
cylc broadcast -s 'script = "foo" '
cylc broadcast -s 'script = foo '
and both set script=foo
@classmethod | ||
def coerce_str(cls, value, keys) -> str: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@classmethod | |
def coerce_str(cls, value, keys) -> str: | |
@staticmethod | |
def coerce_str(value, keys) -> str: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is overriding a class method from ParsecValidator
cylc broadcast --file
with a relative filepath#
character in a broadcast string setting (e.g. the script setting) would be stripped out - edit runtime strips characters after # #5932Check List
CONTRIBUTING.md
and added my name as a Code Contributor.CHANGES.md
entry included if this is a change that can affect users?.?.x
branch.