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

chore: Update Add Transform Tests Script #2984

Merged
merged 3 commits into from
Mar 1, 2023

Conversation

GavinZZ
Copy link
Contributor

@GavinZZ GavinZZ commented Mar 1, 2023

Issue #, if available

Description of changes

Update add transform test script to correctly generate API deployment id.

Description of how you validated changes

Generated a api template with previous and new add_transform_test_script. Confirmed that it works.

Checklist

Examples?

Please reach out in the comments if you want to add an example. Examples will be
added to sam init through aws/aws-sam-cli-app-templates.

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

@GavinZZ GavinZZ requested a review from a team as a code owner March 1, 2023 20:16
@GavinZZ GavinZZ changed the title Update Add Transform Tests Script chore: Update Add Transform Tests Script Mar 1, 2023
Copy link
Contributor

@acristin acristin left a comment

Choose a reason for hiding this comment

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

Seems fine; was there any intentionality behind the previous approach of running the transform in a separate process?

@GavinZZ GavinZZ force-pushed the fix_add_transform_script branch from 0e494cb to 1d76f0a Compare March 1, 2023 20:25
@GavinZZ
Copy link
Contributor Author

GavinZZ commented Mar 1, 2023

Seems fine; was there any intentionality behind the previous approach of running the transform in a separate process?

Yes, we were using a library(I forgot the name) for doc string parsing which somehow messes up importing modules. I had to use a separate process for the script. However, Chris made changes to these scripts to removed that library.

Tested out with multiple templates and I haven't encountered any issue.

@GavinZZ GavinZZ merged commit 25d1e5e into aws:develop Mar 1, 2023
GavinZZ added a commit to GavinZZ/serverless-application-model that referenced this pull request Mar 2, 2023
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