Skip to content

Conversation

MEPalma
Copy link
Contributor

@MEPalma MEPalma commented Jan 19, 2023

Adds base: lexer, grammar, components, and implementation of stepfunctions' intrinsic functions.

Unrelated test failure:
tests/integration/stepfunctions/legacy/test_stepfunctions_legacy.py::test_aws_sdk_task_delete_s3_object

@MEPalma MEPalma self-assigned this Jan 19, 2023
@MEPalma MEPalma temporarily deployed to localstack-ext-tests January 19, 2023 21:45 — with GitHub Actions Inactive
@MEPalma MEPalma temporarily deployed to localstack-ext-tests January 19, 2023 21:50 — with GitHub Actions Inactive
@MEPalma MEPalma temporarily deployed to localstack-ext-tests January 19, 2023 21:54 — with GitHub Actions Inactive
@MEPalma MEPalma temporarily deployed to localstack-ext-tests January 19, 2023 21:57 — with GitHub Actions Inactive
@github-actions
Copy link

github-actions bot commented Jan 19, 2023

LocalStack integration with Pro

       3 files  ±0         3 suites  ±0   1h 34m 28s ⏱️ +45s
1 731 tests ±0  1 382 ✔️ ±0  349 💤 ±0  0 ±0 
2 449 runs  ±0  1 758 ✔️ ±0  691 💤 ±0  0 ±0 

Results for commit 1c30300. ± Comparison against base commit 2a4be3d.

♻️ This comment has been updated with latest results.

@MEPalma MEPalma temporarily deployed to localstack-ext-tests February 1, 2023 15:28 — with GitHub Actions Inactive
@MEPalma MEPalma temporarily deployed to localstack-ext-tests February 6, 2023 09:57 — with GitHub Actions Inactive
@MEPalma MEPalma requested review from giograno and whummer February 6, 2023 09:58
@thrau thrau removed their request for review February 6, 2023 16:50
@whummer
Copy link
Member

whummer commented Feb 6, 2023

@MEPalma Let's review this PR once #7464 is merged and this PR has been rebased.. 👍

@MEPalma MEPalma temporarily deployed to localstack-ext-tests February 13, 2023 14:04 — with GitHub Actions Inactive
@MEPalma MEPalma removed the request for review from giograno February 13, 2023 14:06
@MEPalma MEPalma temporarily deployed to localstack-ext-tests February 14, 2023 19:20 — with GitHub Actions Inactive
@MEPalma MEPalma temporarily deployed to localstack-ext-tests February 18, 2023 11:12 — with GitHub Actions Inactive
@coveralls
Copy link

Coverage Status

Coverage: 85.168% (+0.2%) from 84.971% when pulling 1c30300 on MEP-stepfunctions-intrinsic-v0 into 2a4be3d on master.

Copy link
Member

@dominikschubert dominikschubert left a comment

Choose a reason for hiding this comment

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

LGTM 👍

A few remarks:

  • We can probably remove the number of files a bit by aggregating them which would be a bit more pythonic in general. E.g. Keep all the different function_argument_x classes in function_argument.py. This generally also makes reviewing, code navigation and refactoring in general a bit easier

  • Do you think we should also verify the input argument types more? I'm not sure how stepfunction behaves when you for example try to add or multiply a number + string or when you have an array of non-string values. 🤔

@@ -0,0 +1,27 @@
from enum import Enum
Copy link
Member

Choose a reason for hiding this comment

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

file name has a typo (fuinction => function)

@@ -404,10 +409,10 @@ def check_invocations():
# clean up
cleanup(sm_arn, state_machines_before, stepfunctions_client)

@pytest.mark.skip("Intrinsic Functions not yet supported.")
# @pytest.mark.skip("Intrinsic Functions not yet supported.")
Copy link
Member

Choose a reason for hiding this comment

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

this should probably just be deleted right?

Comment on lines -409 to +415
if os.environ.get("AWS_DEFAULT_REGION") != "us-east-1":
pytest.skip("skipping non us-east-1 temporarily")
# if os.environ.get("AWS_DEFAULT_REGION") != "us-east-1":
# pytest.skip("skipping non us-east-1 temporarily")
Copy link
Member

Choose a reason for hiding this comment

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

same here

@dominikschubert dominikschubert merged commit 8eaabbd into master Feb 22, 2023
@dominikschubert dominikschubert deleted the MEP-stepfunctions-intrinsic-v0 branch February 22, 2023 10:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants