-
Notifications
You must be signed in to change notification settings - Fork 66
feat: workflow support combining fixed strings with variable expansions #2918
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
feat: workflow support combining fixed strings with variable expansions #2918
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files
|
Robot Results
|
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.
Approved (from a functional perspective)
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.
One question, but I'm happy with the Rust code apart from that
.strip_prefix(".payload.") | ||
.and_then(|path| json_excerpt(&self.payload, path)) | ||
.cloned(), | ||
path if path.contains(['[', ']']) => None, |
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.
I'm slightly confused as to what case this is covering? Is it explicitly filtering out TOML table headings? If so, why isn't strip_prefix
covering this case?
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.
The point is to exclude any JSON path where a bracket notation is used to access an item below an array. Such a path is currently considered as ill-formed (the path expression is left unchanged), by contrast with paths leading to an unknown value (the path expression being then substituted for the empty string). See #2918 (comment).
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.
Why is accessing an array value considered invalid? Because array indexes positions are non-deterministic? Or to avoid out-of-bound index risks? If it was the latter, I'd have used the same logic for that as unknown path in an existing root: returning empty.
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.
Why is accessing an array value considered invalid? Because array indexes positions are non-deterministic? Or to avoid out-of-bound index risks? If it was the latter, I'd have used the same logic for that as unknown path in an existing root: returning empty.
Not yet implemented would be a better term. I fully agree that this can be useful.
Signed-off-by: Didier Wenzek <didier.wenzek@free.fr>
3b2d443
to
485b8e2
Compare
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.
LGTM.
.strip_prefix(".payload.") | ||
.and_then(|path| json_excerpt(&self.payload, path)) | ||
.cloned(), | ||
path if path.contains(['[', ']']) => None, |
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.
Why is accessing an array value considered invalid? Because array indexes positions are non-deterministic? Or to avoid out-of-bound index risks? If it was the latter, I'd have used the same logic for that as unknown path in an existing root: returning empty.
|
||
fn inject_values_into_script(state: &GenericCommandState, script: &ShellScript) -> ShellScript { | ||
ShellScript { | ||
command: state.inject_values_into_template(&script.command), |
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.
Jus wondering if we should add some additional restrictions to the variable substitution in script and operation names, limiting the substitution to primitive types only (Strings, numbers etc) and explicitly prevent complex values like arrays and objects as that's most likely an error when used in a script/operation name? It would be better fail during the validation phase itself rather than during execution.
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.
Yeah, why not.
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.
Though this shouldn't prevent passing complex objects as arguments to the script right?
For example:
[executing]
script = "/usr/bin/do_something ${.payload}"
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.
Sure. I think @albinsuresh is speaking only about operation names.
Signed-off-by: Didier Wenzek <didier.wenzek@free.fr>
Signed-off-by: Didier Wenzek <didier.wenzek@free.fr>
Signed-off-by: Didier Wenzek <didier.wenzek@free.fr>
Signed-off-by: Didier Wenzek <didier.wenzek@free.fr>
e5d5bd1
to
f186b3f
Compare
Proposed changes
Full string interpolation for workflow script parameters.
{"x":"X", "y":"Y"}
"prefix-${.payload.x}-separator-${.payload.y}-suffix"
"prefix-X-separator-Y-suffix"
Types of changes
Paste Link to the issue
Checklist
cargo fmt
as mentioned in CODING_GUIDELINEScargo clippy
as mentioned in CODING_GUIDELINESFurther comments