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

Isue115/inline and distributed maps #116

Merged
merged 16 commits into from
Jan 13, 2023
Merged

Isue115/inline and distributed maps #116

merged 16 commits into from
Jan 13, 2023

Conversation

massfords
Copy link
Collaborator

Added two test cases for the new map features.

The schema types could be better.

ChristopheBougere and others added 15 commits July 5, 2022 18:32
BREAKING CHANGE: updated the json path error
# Conflicts:
#	package-lock.json
#	src/checks/json-schema-errors.ts
#	src/validator.ts
tests: added new test for asserting error messages
chore: updated tsconfig to keep js files out of src
# Conflicts:
#	package-lock.json
#	package.json
#	src/__tests__/error-reporting.test.ts
#	src/checks/json-schema-errors.ts
#	src/schemas/paths.json
#	src/validator.ts
# Conflicts:
#	src/checks/json-schema-errors.ts
@@ -0,0 +1,48 @@
{
"Comment": "Using Map state in Distributed mode",
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

the two example files added were taken from the AWS docs

Comment on lines 100 to 102
}))
.filter((v: StateMachineError,i,a)=>a.findIndex(v2=>(v2.schemaError.schemaPath===v.schemaError?.schemaPath && v2.schemaError.instancePath===v.schemaError?.instancePath))===i);
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

unrelated, but I noticed some duplicate error messages in an existing test after this change so this will remove any dupes before returning the errors

Copy link
Owner

Choose a reason for hiding this comment

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

could you add some new lines to make it easier to read? And maybe add a comment to explain it's for error deduplication

Comment on lines 38 to 41
"ItemReader": {
"$comment": "need to improve this type",
"type": "object"
},
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I believe the fields for this are defined in the spec and don't vary. Easy enough to update to define more here.

"ResultSelector": {
"$ref": "paths.json#/definitions/asl_payload_template"
},
"ResultWriter": {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

need better typxingfor the ResultWriter as well. I think these fields are for the s3 bucket to write to.

Copy link
Owner

Choose a reason for hiding this comment

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

could you add a $comment, like you did for ItemReader?

@massfords
Copy link
Collaborator Author

@ChristopheBougere Sorry for the failing build. I think it's a commit message issue. I made some quick changes to the schemas to support the new types

I haven't read through all of the new Map features yet. The examples I've added pass, but the map schema type still needs some work to avoid accepting definitions with both new and old properties.

Copy link
Owner

@ChristopheBougere ChristopheBougere left a comment

Choose a reason for hiding this comment

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

Thanks for working on this and for the comments @massfords!

A few comments, let me know what you think

FYI you can run a git rebase -i to rewrite your commit messages (or squash them). Otherwise, I'll squash them in github when merging the PR ;)

Also, is it ready to merge or do you want to work more on the ItemReader and ResultWriter before?

@@ -0,0 +1,23 @@
{
Copy link
Owner

Choose a reason for hiding this comment

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

Could you update the state-machine.json schema so it extends this base to avoid duplication?
If I understand well, it is the same without Version and TimeoutSeconds properties?
Also, I think we could use this base in the Branches properties of the parallel.json schema instead of using state-machine.json?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I recall trying this and hitting issues with additionalProperties. I think it's possible to get right with JSON Schema + AJV, I just didn't have the time to work through the issue. My approach was to have the base as its own file and then have state-machine.json reference it with an $allOf and then add its own properties.

"ResultSelector": {
"$ref": "paths.json#/definitions/asl_payload_template"
},
"ResultWriter": {
Copy link
Owner

Choose a reason for hiding this comment

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

could you add a $comment, like you did for ItemReader?

Comment on lines 100 to 102
}))
.filter((v: StateMachineError,i,a)=>a.findIndex(v2=>(v2.schemaError.schemaPath===v.schemaError?.schemaPath && v2.schemaError.instancePath===v.schemaError?.instancePath))===i);
}
Copy link
Owner

Choose a reason for hiding this comment

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

could you add some new lines to make it easier to read? And maybe add a comment to explain it's for error deduplication

@massfords
Copy link
Collaborator Author

I'll have some time this weekend to clean up the PR and address the comments.

@massfords massfords closed this Jan 6, 2023
@massfords massfords reopened this Jan 6, 2023
@massfords
Copy link
Collaborator Author

sorry, didn't mean to close the PR :(

@massfords
Copy link
Collaborator Author

@ChristopheBougere I've made a couple of changes to address the comments.

Please do squash my commits in the merge. I didn't realize how out of date my branch was.

@ChristopheBougere ChristopheBougere merged commit 74772f7 into ChristopheBougere:main Jan 13, 2023
@github-actions
Copy link

🎉 This PR is included in version 3.1.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

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.

Add support for new Distributed Map in Step Functions Definition
2 participants