-
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): allow user to configure log retention time #21418
Conversation
I have a problem executing the integration tests – it doesn't recognize my AWS account credential configuration and thus fails to deploy the test stack. I tried to look around but couldn't find any hints on how the environment should be configured to make these tests work. So, what should I do? As you can see, the CDK thinks my account and region are unknown:
|
03273a0
to
0fa8467
Compare
Hi @nikovirtala and thanks for looking into the integration tests! It depends a bit on how your credentials are configured locally, but I reckon the easiest way to influence the behaviour of running the test, will be env vars. E.g. you can run it like this:
There are a few more variables you can set depending on your exact setup. Please let me know if that helps. |
@nikovirtala When I run the new integration test, the deployment fails with "The fieldLogLevel is invalid". I believe this is a bug in the AppSync control plane. The documentation says that "LogConfig": {
"CloudWatchLogsRoleArn": {
"Fn::GetAtt": [
"GraphqlApiApiLogsRoleBB9E6BAD",
"Arn"
]
}
} Despite being a bug in the backend, we can default the value of this field to |
Pull request has been modified.
I noticed that too but was focused to get the integration tests to run against my environment. It is fixed now. |
If you have too much trouble getting the tests to run, we can run it for you and have you double check that the output is what you'd expect. Granted, it's much better to get your setup working so that you can run tests in the future as well, but feel free to @ me if you need some additional guidance/assistance on that. |
@nikovirtala If you haven't already done so, follow these instructions for your local setup Configuring the AWS CLI. |
d102d3f
to
11291b0
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.
Just a few small comments from me inline.
@TheRealAmazonKendra It's not a huge problem for me to spend some time to figure out why I cannot make the tests run – I seriously doubt that this won't be the last time I need to touch 'em here or on some other package 😉 @mrgrain gave me some ideas on what to try next at cdk.dev Slack yesterday ... going to try those next. |
11291b0
to
7a96bdc
Compare
Pull request has been modified.
Pull request has been modified.
FWIW, I ran the integ tests on my machine, out of curiosity, and they all passed. |
61811bf
to
b8979b4
Compare
Pull request has been modified.
@Mergifyio update |
☑️ Nothing to do
|
I know the test I added passes in its current state but IMO such a test without an assertion doesn't make much sense 😄 Now that I know how to successfully deploy the changed tests, I can add the assertion. |
01026db
to
e198871
Compare
e198871
to
46fc710
Compare
The integration test result is now verified using |
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). |
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). |
This pull request implements a feature that allows users to configure the log retention period for App Sync logs. AWS AppSync doesn't allow users to use user-defined log groups, leaving the option to create the log group for the user and set the retention time accordingly. Fortunately, the service always creates its log groups according to a pre-defined naming convention. > The log group is named following the /aws/appsync/apis/{graphql_api_id} format ref. https://docs.aws.amazon.com/appsync/latest/devguide/monitoring.html At the same time, AWS CDK provides a stable [construct](https://docs.aws.amazon.com/cdk/api/v2/docs/aws-cdk-lib.aws_logs.LogRetention.html) to set log retention times for any log group. Thus, it is completely unnecessary to force every user to learn this detail when it can be abstracted to the construct creating the resource for them – that is what this pull request aims to do. This is the continuation of work done in aws#20536 – this time with documentation and integration test 😄 ---- ### All Submissions: * [x] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) ### Adding new Unconventional Dependencies: * [ ] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md/#adding-new-unconventional-dependencies) ### New Features * [x] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)? * [x] Did you use `yarn integ` to deploy the infrastructure and generate the snapshot (i.e. `yarn integ` without `--dry-run`)? *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
This pull request implements a feature that allows users to configure the log retention period for App Sync logs.
AWS AppSync doesn't allow users to use user-defined log groups, leaving the option to create the log group for the user and set the retention time accordingly. Fortunately, the service always creates its log groups according to a pre-defined naming convention.
ref. https://docs.aws.amazon.com/appsync/latest/devguide/monitoring.html
At the same time, AWS CDK provides a stable construct to set log retention times for any log group.
Thus, it is completely unnecessary to force every user to learn this detail when it can be abstracted to the construct creating the resource for them – that is what this pull request aims to do.
This is the continuation of work done in #20536 – this time with documentation and integration test 😄
All Submissions:
Adding new Unconventional Dependencies:
New Features
yarn integ
to deploy the infrastructure and generate the snapshot (i.e.yarn integ
without--dry-run
)?By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license