Skip to content
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

(core) add appflow stage #68

Merged
merged 1 commit into from
Mar 31, 2022
Merged

(core) add appflow stage #68

merged 1 commit into from
Mar 31, 2022

Conversation

cnfait
Copy link
Contributor

@cnfait cnfait commented Mar 28, 2022

Feature or Bugfix

  • Feature

Detail

Relates

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@cnfait cnfait self-assigned this Mar 28, 2022
@malachi-constant malachi-constant added this to the DDK Stages milestone Mar 28, 2022
@cnfait cnfait force-pushed the appflow-stage branch 2 times, most recently from 766792d to 6c436c7 Compare March 29, 2022 11:58
@cnfait cnfait added core enhancement New feature or request effort/medium labels Mar 29, 2022
@cnfait cnfait force-pushed the appflow-stage branch 6 times, most recently from 41f5c53 to 4fe4ee9 Compare March 31, 2022 10:30
@cnfait cnfait marked this pull request as ready for review March 31, 2022 10:31
Copy link
Contributor

@kukushking kukushking left a comment

Choose a reason for hiding this comment

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

I'll let other folks comment but overall looks good to me!

)

# Create start flow step function task
flow_object = CustomState(
Copy link
Contributor

Choose a reason for hiding this comment

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

One idea is to make the constructor shorter and more readable perhaps we should move some code into private methods e.g. _create_start_flow_custom_task(flow_name: str, ...), etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've done this for the custom state task and the lambda task!

Copy link
Contributor

Choose a reason for hiding this comment

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

Do you need the variables like flow_object in _create_start_flow_custom_task? I think you can just return CustomState(...)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done 👍

core/aws_ddk_core/stages/appflow.py Outdated Show resolved Hide resolved
core/aws_ddk_core/stages/appflow.py Outdated Show resolved Hide resolved
core/aws_ddk_core/stages/appflow.py Outdated Show resolved Hide resolved
core/aws_ddk_core/stages/appflow.py Outdated Show resolved Hide resolved
core/aws_ddk_core/stages/__init__.py Show resolved Hide resolved
@cnfait cnfait force-pushed the appflow-stage branch 2 times, most recently from 06d8052 to 7548e9c Compare March 31, 2022 11:37
@cnfait cnfait force-pushed the appflow-stage branch 3 times, most recently from 09266f6 to 66bcc72 Compare March 31, 2022 15:08
@jaidisido jaidisido merged commit 5e3dbae into main Mar 31, 2022
@jaidisido jaidisido deleted the appflow-stage branch March 31, 2022 16:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants