Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

Add support for Amazon States Language "ResultSelector" in Task, Map …#102

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Closed
yoodan93 wants to merge15 commits intoaws:mainfromyoodan93:master

Conversation

@yoodan93
Copy link
Contributor

@yoodan93yoodan93 commentedOct 29, 2020•
edited by ca-nguyen
Loading

Add ResultSelector support for Task, Map and Parallel states.

Issue #, if available:
#95

Description of changes

Adding new Amazon States Language payload template "ResultSelector"https://states-language.net/#payload-template
This change allows user to define aresult_selector which is used to define the effective result of a state.

Testing

  • Added unit test for changes
  • Manual test:
# Create a simple workflow containing a task state with `result_selector`from stepfunctions.inputs import StepResultstep_result = StepResult()step_result_placeholder = {    'Output': step_result['output']}state_with_result_selector = Task(    state_id='StartState',    parameters={'Key': 'Value'},    result_selector=step_result_placeholder,    resource=<resource_arn>)workflow = Workflow(    name="ResultSelector_Example",    definition=state_with_result_selector,    role=<workflow_execution_role>)# Execute the workflow and confirm it has completed successfullyworkflow.execute()

Workflow definition:

{    "StartAt": "StartState",    "States": {        "StartState": {            "Parameters": {                "Key": "Value"            },            "ResultSelector": {                "Output.$": "$['output']"            },            "Resource": <resource_arn>,            "Type": "Task",            "End": true        }    }}

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

a13zen, milesgranger, creatorrr, nicolaei, and lasdem reacted with heart emoji
@StepFunctions-Bot
Copy link
Contributor

AWS CodeBuild CI Report

  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered bygithub-codebuild-logs, available on theAWS Serverless Application Repository

@StepFunctions-Bot
Copy link
Contributor

AWS CodeBuild CI Report

  • CodeBuild project: StepFunctionsPythonSDK-integtests
  • Commit ID:41f2db2
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered bygithub-codebuild-logs, available on theAWS Serverless Application Repository

Base automatically changed frommaster tomainFebruary 25, 2021 21:10
@wong-awong-a linked an issueApr 30, 2021 that may beclosed by this pull request
wong-a
wong-a previously requested changesApr 30, 2021
ca-nguyen
ca-nguyen previously approved these changesAug 11, 2021
@milesgranger
Copy link

Any update with this? Would be fantastic to have! 👍

shivlaks
shivlaks previously requested changesSep 2, 2021
Copy link
Contributor

@shivlaksshivlaks left a comment

Choose a reason for hiding this comment

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

nice work@yoodan93 !!

overall approach looks good. had some minor/naive questions.
I do think we can raise the bar on testing a bit more though

@ca-nguyen
Copy link
Contributor

Taking over to help merge@yoodan93 's PR
Thank you for your review - will be addressing comments shortly

milesgranger, shivlaks, and lasdem reacted with rocket emoji

Copy link
Contributor

@shivlaksshivlaks left a comment

Choose a reason for hiding this comment

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

can you also add some details around testing - i.e.

  • have we tried generating a workflow and executing the state machine successfully?

at some point we should probably add some basic integ tests that exercise state machines and their support for ASL features. Presumably, we'll be adding more capabilities in the future. Probably not in scope for this PR, but worth thinking about sooner than later.

@ca-nguyen
Copy link
Contributor

can you also add some details around testing

Added manual test details

at some point we should probably add some basic integ tests that exercise state machines and their support for ASL features. Presumably, we'll be adding more capabilities in the future. Probably not in scope for this PR, but worth thinking about sooner than later.

Agreed! let's do add integ tests in another PR

@ca-nguyenca-nguyen dismissed stale reviews fromshivlaks andwong-aSeptember 15, 2021 23:25

Requested changes have been addressed in the last commits

@wong-a
Copy link
Contributor

Manual test: Generated a workflow with a Task with StepResult placeholder and executed successfully

This is very unspecific. Can you share the SDK code you used to generate that?

comment (str, optional): Human-readable comment or description. (default: None)
input_path (str, optional): Path applied to the state’s raw input to select some or all of it; that selection is used by the state. (default: '$')
parameters (dict, optional): The value of this field becomes the effective input for the state.
result_selector (dict, optional): The value of this field becomes the effective result of the state.
Copy link
Contributor

Choose a reason for hiding this comment

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

Maintaining these docstrings for every inheriting class is pretty annoying. I wonder if there anything we can do to make that easier.

Step Result
-----------
The third mechanism is a placeholder for a step's result. The result of a step can be modified
Copy link
Contributor

Choose a reason for hiding this comment

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

Still wondering if it's really necessary to introduce StepResult when StepInput could also be used to achieve the same thing. I left that comment earlier but it didn't get an answer. Literally the only difference is the class name. Thoughts@shivlaks?

We can always add classes later, but removing it afterwards breaks back-compat.

lambda_result=StepInput(schema={"Id":str,    })lambda_state_first=LambdaStep(state_id="MyFirstLambdaStep",result_selector={"Output":lambda_result["Id"],"Status":"Success"    })

Copy link
Contributor

Choose a reason for hiding this comment

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

Good question - I missed that comment earlier

While it does make sense to use the same object, I think the nameStepInput could bring confusion since, in this case, we are not using the step's input, but the step's result (or output)

To be backwards compatible, we can't renameStepInput to a more general term. Alternatively, we could makeStepResult inherit fromStepInput instead ofPlaceholder and avoid code duplication.

@ca-nguyen
Copy link
Contributor

This is very unspecific. Can you share the SDK code you used to generate that?

Agreed - this is not sufficient information to reproduce the test. I provided more details in theTesting section of the PR description

@StepFunctions-Bot
Copy link
Contributor

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject6AEA49D1-sEHrOdk7acJc
  • Commit ID:6594b1c
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered bygithub-codebuild-logs, available on theAWS Serverless Application Repository

@alesageProovStation

Hi! Any plans on merging this PR soon? I'm writing a stepfunction state machine which requires this... Thanks.

v-weh and asgutierrez reacted with thumbs up emoji

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@shivlaksshivlaksAwaiting requested review from shivlaks

@wong-awong-aAwaiting requested review from wong-a

1 more reviewer

@ca-nguyenca-nguyenca-nguyen left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Feature Request: Add support for ResultSelector

8 participants

@yoodan93@StepFunctions-Bot@milesgranger@ca-nguyen@wong-a@alesageProovStation@shivlaks@evscott
[8]ページ先頭
©2009-2025 Movatter.jp