-
Notifications
You must be signed in to change notification settings - Fork 4k
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
feat(applicationsignals-alpha): introduce Application Signals L2 constructs #32931
base: main
Are you sure you want to change the base?
Conversation
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 pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.
A comment requesting an exemption should contain the text Exemption Request
. Additionally, if clarification is needed add Clarification Request
to a comment.
✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #32931 +/- ##
=======================================
Coverage 82.21% 82.21%
=======================================
Files 119 119
Lines 6876 6876
Branches 1162 1162
=======================================
Hits 5653 5653
Misses 1120 1120
Partials 103 103
Flags with carried forward coverage won't be shown. Click here to find out more.
|
921a486
to
43766a8
Compare
@Mergifyio refresh |
✅ Pull request refreshed |
652b1da
to
021dcf9
Compare
throw new Error('Fargate tasks must deploy CloudWatch Agent as a sidecar container'); | ||
} | ||
if (cloudWatchAgentConfig.enableSidecar) { | ||
props.taskDefinition.taskRole.addManagedPolicy(ManagedPolicy.fromAwsManagedPolicyName('CloudWatchAgentServerPolicy')); |
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.
using this Managed policy violates the Apply least-privilege permissions rule of the security best practices, since as defined here, this managed policy enable multiple actions for *
resources. It will be petter to add a new Policy, and add the specific resources that you need these actions to be allowed on them.
packages/@aws-cdk/aws-applicationsignals-alpha/lib/enablement/ecs.ts
Outdated
Show resolved
Hide resolved
021dcf9
to
6a0863c
Compare
Pull request has been modified.
5a2f537
to
f8b8fa2
Compare
This PR has been in the BUILD FAILING state for 3 weeks, and looks abandoned. Note that PRs with failing linting check or builds are not reviewed, please ensure your build is passing To prevent automatic closure:
This PR will automatically close in 7 days if no action is taken. |
This PR has been deemed to be abandoned, and will be automatically closed. Please create a new PR for these changes if you think this decision has been made in error. |
Comments on closed issues and PRs are hard for our team to see. |
@bjrara .. are you planning to finalize this PR ? |
Yes. |
9555b78
to
e80aad4
Compare
e80aad4
to
bb1e7ce
Compare
bb1e7ce
to
612770e
Compare
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
Issue # (if applicable)
N/A
Description of changes
This PR adds L2 constructs to simplify the Application Signals enablement process. See aws/aws-cdk-rfcs#672 for more details.
Describe any new or updated permissions being added
Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license