-
Notifications
You must be signed in to change notification settings - Fork 4k
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): add x-ray parameter to AppSync #9389
Conversation
Title does not follow the guidelines of Conventional Commits. Please adjust title before merge. |
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 contribution @tomoki10!!
Just a couple minor things before I'm ready to ship this out 😊
@@ -383,6 +389,7 @@ export class GraphQLApi extends Construct { | |||
) | |||
: undefined, | |||
additionalAuthenticationProviders: this.formatAdditionalAuthenticationProviders(props), | |||
xrayEnabled: props.xrayEnabled || false, |
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 property doesn't need to be configured here. If it is undefined in CloudFormations, Cfn will configure the AppSync without x-ray support by default.
xrayEnabled: props.xrayEnabled || false, | |
xrayEnabled: props.xrayEnabled, |
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.
@BryanPan342
Thank you for your comment.
I will fix it along with the integration test.
``` | ||
|
||
See the [the AWS documentation](https://docs.aws.amazon.com/appsync/latest/devguide/x-ray-tracing.html) to learn more about AWS AppSync's X-Ray support. |
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 would rather us either put a description of what x-ray support is or not include it at all in the README.
Since it is a feature, I think in order to avoid the linter error, I would be okay with just adding the prop to the example
section.
Like as follows:
const api = new appsync.GraphQLApi(stack, 'Api', {
name: 'demo',
schemaDefinition: appsync.SchemaDefinition.FILE,
schemaDefinitionFile: join(__dirname, 'schema.graphql'),
authorizationConfig: {
defaultAuthorization: {
authorizationType: appsync.AuthorizationType.IAM
},
},
xrayEnabled: true,
});
@MrArnoldPalmer wdyt?
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'll fix it as commented.
Pull request has been modified.
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.
LGTM 🎉
Thanks for the contribution @tomoki10!!
Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
@tomoki10 looks like the error is the following:
This is coming from the breaking change that we have in the works at the moment. If you just add a prop similar to the line above. We should be good to go. aws-cdk/packages/@aws-cdk/aws-appsync/test/appsync.test.ts Lines 29 to 34 in 3ad8e8e
|
Pull request has been modified.
Pull request has been modified.
Pull request has been modified.
Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
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 master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
Close: #7640 - [x] Adding a boolean prop for xrayEnabled - [x] Linking it to the xrayEnabled prop in class CfnGraphQLApi from appsync.generated.ts file that is generated on yarn build - [x] Writing a unit test to check whether that the boolean property is set in the CloudFormation Template ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Close: #7640
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license