Skip to content

Conversation

didier-wenzek
Copy link
Contributor

@didier-wenzek didier-wenzek commented Jun 4, 2024

Proposed changes

Full string interpolation for workflow script parameters.

  • in a context where the operation instance payload is {"x":"X", "y":"Y"}
  • the template "prefix-${.payload.x}-separator-${.payload.y}-suffix"
  • is replaced by "prefix-X-separator-Y-suffix"

Types of changes

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Improvement (general improvements like code refactoring that doesn't explicitly fix a bug or add any new functionality)
  • Documentation Update (if none of the other choices apply)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Paste Link to the issue

Checklist

  • I have read the CONTRIBUTING doc
  • I have signed the CLA (in all commits with git commit -s)
  • I ran cargo fmt as mentioned in CODING_GUIDELINES
  • I used cargo clippy as mentioned in CODING_GUIDELINES
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)

Further comments

Copy link

codecov bot commented Jun 4, 2024

Codecov Report

Attention: Patch coverage is 90.90909% with 24 lines in your changes missing coverage. Please review.

Project coverage is 78.0%. Comparing base (7d57cad) to head (f186b3f).
Report is 13 commits behind head on main.

Additional details and impacted files
Files Coverage Δ
crates/core/tedge_api/src/workflow/script.rs 92.3% <98.4%> (+11.2%) ⬆️
crates/core/tedge_api/src/workflow/state.rs 74.8% <96.5%> (+5.1%) ⬆️
crates/core/tedge_api/src/workflow/mod.rs 58.0% <0.0%> (-2.6%) ⬇️

... and 15 files with indirect coverage changes

Copy link
Contributor

github-actions bot commented Jun 4, 2024

Robot Results

✅ Passed ❌ Failed ⏭️ Skipped Total Pass % ⏱️ Duration
452 0 3 452 100 1h13m17.055406999s

Copy link
Contributor

@reubenmiller reubenmiller left a 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)

Copy link
Contributor

@jarhodes314 jarhodes314 left a 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,
Copy link
Contributor

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?

Copy link
Contributor Author

@didier-wenzek didier-wenzek Jun 6, 2024

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).

Copy link
Contributor

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.

Copy link
Contributor Author

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>
@didier-wenzek didier-wenzek force-pushed the feat/workflow-support-combining-fixed-strings-with-variable-expansions branch from 3b2d443 to 485b8e2 Compare June 6, 2024 12:51
Copy link
Contributor

@albinsuresh albinsuresh left a 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,
Copy link
Contributor

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),
Copy link
Contributor

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, why not.

Copy link
Contributor

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}"

Copy link
Contributor Author

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>
@didier-wenzek didier-wenzek force-pushed the feat/workflow-support-combining-fixed-strings-with-variable-expansions branch from e5d5bd1 to f186b3f Compare June 6, 2024 15:12
@didier-wenzek didier-wenzek temporarily deployed to Test Pull Request June 6, 2024 15:12 — with GitHub Actions Inactive
@didier-wenzek didier-wenzek added this pull request to the merge queue Jun 6, 2024
Merged via the queue into thin-edge:main with commit 5200b74 Jun 6, 2024
@reubenmiller reubenmiller added the theme:workflows Theme: Workflow engine topics label Jun 6, 2024
@didier-wenzek didier-wenzek deleted the feat/workflow-support-combining-fixed-strings-with-variable-expansions branch June 6, 2024 15:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
theme:workflows Theme: Workflow engine topics
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants