-
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
feat: Support placeholders for input_path and output_path for all States (except Fail) and items_path for MapState #158
base: main
Are you sure you want to change the base?
Conversation
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.
thanks for putting this PR together! left some feedback, let me know if you have any questions.
some higher level thoughts:
- I think we should also update the docs related to placeholders. Might make sense to re-purpose the example in your commit body to illustrate its usage.
- does it make sense to add an integration test that exercises placeholders? as we expand the use cases and scenarios we support, I'm thinking it would be useful to have some basic integ tests as unit tests are more brittle.
} | ||
) | ||
|
||
map_state.attach_iterator(iterator_state) |
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.
question: what happens with this? - does it become part of the workflow definition somewhere? I thought the Iterator
and ItemsPath
properties were required for Map
states
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.
ItemsPath is optional. Similar to InputPath and OutputPath, the default is $
.
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 iterator is added to the Map State here and will be added to the workflow definition when the Map state itself is added to the workflow definition
The docstring will need to be updated - it seems like iterator
is required in the Map State constructor when in fact, it gets creates successfully without it. However, the workflow will fail to create if no iterator is set - we could add a validation here and raise an exception if the iterator is not set. At the moment, if fails here with the following message
ValueError: Expected branch to be a State or a Chain, but got `None`
This can be done in another PR to avoid making the scope larger
} | ||
) | ||
|
||
map_state.attach_iterator(iterator_state) |
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.
ItemsPath is optional. Similar to InputPath and OutputPath, the default is $
.
Clarified what "support placeholders for Map state" means in the initial issue comments: #101 (comment). So far, this PR only addresses the 3rd use case, which isn't actually what the requester was trying to do. |
Agreed - will update the placeholder docs
Yes it does - will include one for Map state |
@ca-nguyen This change doesn't just affect Map state. There's the 3 things I mentioned here: #101 (comment) InputPath and OutputPath are allowed in all state types except Fail. Please update the PR title, description, and tests accordingly. |
…cstring and tests for all states that support input_path and output_path
Co-authored-by: Adam Wong <55506708+wong-a@users.noreply.github.com>
Updated the PR title and description |
Requested changes were addressed in the last commits
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.
Not finished reviewing, but here are some comments. Implementation seems fine at a quick glance.
Co-authored-by: Adam Wong <55506708+wong-a@users.noreply.github.com>
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
Description
With this change, it will be possible to use:
Fixes #101
Why is the change necessary?
This enables the capacity to define input_path and output_path values dynamically for all States (except Fail State). This also supports using placeholder for items_path and context object for MapState.
Solution
Support Placeholders for input_path, output_path and items_path
During workflow definition serialization, replace placeholder with json path when the parsed argument is one of the three (input_path, output_pat, items_path).
Support Context Object Data for Map State
Add new Placeholder objects MapItemValue and MapItemIndex with a json string template to use during workflow definition serialization.
$$.Map.Item.Value{}
$$.Map.Item.Index
Example
Workflow definition will be:
Testing
Pull Request Checklist
Please check all boxes (including N/A items)
Testing
Documentation
Title and description
Fixes #xxx
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license.