-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
feat(appsync): environmentVariables
property for GraphqlApi
#29064
Conversation
import
9eefe98
to
ccc1b72
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @go-to-k! I just have one question really. the rest looks good
} | ||
|
||
private validateEnvironmentVariables() { | ||
this.node.addValidation( { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why add validation to the node rather than just throwing an error here? I'm not too familiar with this method but usually we want to default to fail as early as possible and i wonder if this violates that. did you see this done elsewhere?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The addValidation
(see DESIGN_GUIDELINES) method is a lazy way to delay and make decisions about processing that cannot be determined during initialization by the constructor.
For example, in this case, we get an array-type property from props and initialize an instance variable in the constructor. However, there are cases where users want to pass nothing in props and add it later with the addEnvironmentVariable
method. In that case, if we check the number of elements in the array in the constructor, it will be assumed to be zero.
Because of this case, we should use this method for lazy checking instead of early checking at the constructor.
Also, if we were to check the number of elements in the array in the addEnvironmentVariable
method, it would throw an error the moment the maximum number is reached, so even if users specify 60 elements for the array, they would get a message Only 50 environment variables can be set, got 51
. The addValidation
method is also necessary to check exactly how many elements the user has specified for the array.
Elsewhere, I saw this in CodePipeline.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made it a little clearer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for the clarification. i agree that this is the right thing to do here now
Thanks for your review. I have returned your comment and would like you to confirm it. |
This looks great. can you add a check to make sure that environment variables are not set on a MERGED api? |
Thanks, nice catch. I will add the change not to set the environment variables on the Merged API. |
if (this.definition.sourceApiOptions) { | ||
throw new Error('Environment variables are not supported for merged APIs'); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because Environment variables are not supported for merged API. The Cfn errors:
MergedAPI (MergedAPI08D3EAD1) Operation not allowed, Merged API resources are read-only (Service: AmazonDeepdish; Status Code: 400; Error Code: BadRequestException; Request ID: ...
I have also added new commits, could you please check them? |
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
Thanks for your approval, but the merge has failed because of Could you please trigger again? |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
Reason for this change
AppSync now supports environment variables in GraphQL resolvers and functions. It would be good for
GraphqlApi
construct to have the property.Description of changes
The
environmentVariables
property is added toGraphqlApi
construct. To add environment variables after the initiation, we can useaddEnvironmentVariables
method.Description of how you validated changes
Both unit and integ tests.
Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license