Skip to content

refactor: improve handling of workflow default values #3064

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 6 commits into from
Aug 21, 2024
Merged

refactor: improve handling of workflow default values #3064

merged 6 commits into from
Aug 21, 2024

Conversation

didier-wenzek
Copy link
Contributor

@didier-wenzek didier-wenzek commented Aug 14, 2024 •
edited
Loading

Proposed changes

Inject workflow default values while parsing the workflow definition and not during execution.
When a workflow is used to execute an operation, all the transitions must have been resolved.

  • Provide sensible defaults for the DefaultHandlers of a workflow
  • Impl TryFrom<(TomlExitHandlers, DefaultHandlers)> for Handlers
  • Deprecate the with_default methods from Handlers
  • Remove Option types from handlers mandatory fields
  • Remove on_success from the DefaultHandlers.

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

@codecov Codecov
Copy link

codecov bot commented Aug 14, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 86.30137% with 30 lines in your changes missing coverage. Please review.

Files Patch % Lines
crates/core/tedge_api/src/workflow/toml_config.rs 58.62% 23 Missing and 1 partial ⚠️
.../core/tedge_agent/src/operation_workflows/actor.rs 0.00% 3 Missing ⚠️
crates/core/tedge_api/src/workflow/script.rs 97.91% 3 Missing ⚠️
Additional details and impacted files

📢 Thoughts on this report? Let us know!

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Aug 14, 2024 •
edited
Loading

Robot Results

✅ Passed ❌ Failed ⏭️ Skipped Total Pass % ⏱️ Duration
495 0 2 495 100 1h38m13.618884s

@didier-wenzek didier-wenzek self-assigned this Aug 16, 2024
albinsuresh
albinsuresh approved these changes Aug 20, 2024
View reviewed changes
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

@didier-wenzek
Provide sensible defaults for the DefaultHandlers of a workflow
ac8142a 
Signed-off-by: Didier Wenzek <didier.wenzek@free.fr>
@didier-wenzek
impl TryFrom<(TomlExitHandlers, DefaultHandlers)> for Handlers
760b0cb 
With the previous impl TryFrom<TomlExitHandlers> for Handlers many
definition consistency and completness checks were not feasible,
the default values being injected only once the struct fully built.

This commit is a preparation step.
The next step is to deprecate the `with_default` method,
moving its effects into to the TryFrom implementations.

Signed-off-by: Didier Wenzek <didier.wenzek@free.fr>
@didier-wenzek
Deprecate the with_default methods from Handlers
b646afb 
Signed-off-by: Didier Wenzek <didier.wenzek@free.fr>
@didier-wenzek
Remove Option types from AwaitHandlers next states
6186d8b 
The aim is to resolve all the workflow transitions when the workflow
definition is parsed and not during its execution.

Signed-off-by: Didier Wenzek <didier.wenzek@free.fr>
@didier-wenzek
Remove on_success from DefaultHandlers
dfb916d 
The idea is to force the users to be explicit on which state is to be
used on a successful action. Using the terminal successful test being
likely an error on most cases.

Signed-off-by: Didier Wenzek <didier.wenzek@free.fr>
@didier-wenzek didier-wenzek force-pushed the refactor/workflow-default-values branch from ac54e96 to dfb916d Compare August 21, 2024 07:48
@didier-wenzek
Remove Option from IterateHandlers::on_error
70edb0c 
Signed-off-by: Didier Wenzek <didier.wenzek@free.fr>
albinsuresh
albinsuresh approved these changes Aug 21, 2024
View reviewed changes
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

@didier-wenzek didier-wenzek added this pull request to the merge queue Aug 21, 2024
Merged via the queue into thin-edge:main with commit 162f9a8 Aug 21, 2024
33 checks passed
@reubenmiller reubenmiller added the theme:workflows Theme: Workflow engine topics label Aug 27, 2024
@didier-wenzek didier-wenzek deleted the refactor/workflow-default-values branch September 4, 2024 07:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@albinsuresh albinsuresh albinsuresh approved these changes

@jarhodes314 jarhodes314 Awaiting requested review from jarhodes314 jarhodes314 is a code owner

@rina23q rina23q Awaiting requested review from rina23q rina23q is a code owner

Assignees

@didier-wenzek didier-wenzek

Labels
theme:workflows Theme: Workflow engine topics
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@didier-wenzek @albinsuresh @reubenmiller