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

feat: make Logging property for GraphQLApi #2971

Merged
merged 4 commits into from
Mar 1, 2023
Merged

feat: make Logging property for GraphQLApi #2971

merged 4 commits into from
Mar 1, 2023

Conversation

pradrx
Copy link
Contributor

@pradrx pradrx commented Feb 27, 2023

Issue #, if available

Description of changes

Created new property Logging in Serverless::GraphQLApi resource, which allows customers to specify their logging implementation in the SAM template.

Maps to the LogConfig property of the AppSync::GraphQLApi resource. Includes defaults for 2 properties:

  • CloudWatchLogsRoleArn: If not defined, creates a role with an AWS managed AppSync CloudWatch policy and attaches to the GraphQLApi resource.
  • FieldLogLevel: Defaults to ALL.

This change also includes a small refactor of the error transform tests for GraphQLApi, bundling them all in error_graphqlapi.yaml.

Description of how you validated changes

Modified and created transform tests to work with Logging property.

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.

@pradrx pradrx requested a review from a team as a code owner February 27, 2023 23:45
Copy link
Contributor

@GavinZZ GavinZZ left a comment

Choose a reason for hiding this comment

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

Overall looks good. Just some nit comments.

samtranslator/model/sam_resources.py Outdated Show resolved Hide resolved
samtranslator/model/sam_resources.py Outdated Show resolved Hide resolved
samtranslator/internal/model/appsync.py Show resolved Hide resolved
samtranslator/model/sam_resources.py Show resolved Hide resolved
samtranslator/model/sam_resources.py Outdated Show resolved Hide resolved
samtranslator/model/sam_resources.py Outdated Show resolved Hide resolved
samtranslator/model/sam_resources.py Outdated Show resolved Hide resolved
samtranslator/model/sam_resources.py Outdated Show resolved Hide resolved
tests/translator/input/error_graphqlapi.yaml Show resolved Hide resolved
@pradrx
Copy link
Contributor Author

pradrx commented Feb 28, 2023

The most recent commits should address the majority of comments that were focused on type checking and reducing branching.

I separated the logic for when Logging property is a boolean type or a dictionary type, so they now have two separate functions: create_logging_default() and parse_logging_properties().

This reduced the nested if branches and repeated type checks, and also separates the logic in what should be a more readable manner.

GavinZZ
GavinZZ previously approved these changes Feb 28, 2023
samtranslator/model/sam_resources.py Outdated Show resolved Hide resolved
ssenchenko
ssenchenko previously approved these changes Feb 28, 2023
hoffa
hoffa previously approved these changes Feb 28, 2023
samtranslator/model/sam_resources.py Show resolved Hide resolved
@hoffa hoffa self-requested a review February 28, 2023 21:25
aaythapa
aaythapa previously approved these changes Mar 1, 2023
hoffa
hoffa previously approved these changes Mar 1, 2023
@pradrx pradrx dismissed stale reviews from hoffa, aaythapa, ssenchenko, and GavinZZ via 0bc8f53 March 1, 2023 01:38
@ssenchenko ssenchenko merged commit d78e92f into aws:feat-appsync Mar 1, 2023
GavinZZ pushed a commit to GavinZZ/serverless-application-model that referenced this pull request Mar 2, 2023
GavinZZ pushed a commit to GavinZZ/serverless-application-model that referenced this pull request Mar 2, 2023
GavinZZ pushed 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.

6 participants