-
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(cloudwatch): Add CustomWidget #19327
Conversation
7a24218
to
e02ea5a
Compare
Hi @markussiebert, following this PR with much interest. Have you requested a review from @madeline-k yet? You should be able to click Madeline's avatar just to the right of your original comment on the PR to request a review. |
Hi @knightjoel - I can't - something I am struggling with. I don't know where to click. But I've already written with @madeline-k on cdk.dev and she promised to review both of my CW PRs. I think she is busy at the moment?! |
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.
This looks pretty good, I am still looking for documentation of the cloudwatch API for all the properties of custom widgets.
Co-authored-by: Madeline Kusters <80541297+madeline-k@users.noreply.github.com>
Pull request has been modified.
Co-authored-by: Madeline Kusters <80541297+madeline-k@users.noreply.github.com>
…rt/aws-cdk into feature/CustomWidget
…rt/aws-cdk into feature/CustomWidget
Hi @markussiebert, still following with much interest. Is this ready for another review? Have all of Madeline's comments been addressed? |
I hope so :-) I pinged her via cdk.dev - maybe she will review this again - the review request here is open since some time |
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.
Looks good, @markussiebert! 😄
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). |
This PR introcudes the CustomWidget for Cloudwatch Dashboards fixes: aws#17579 based on the work of @dannyber If you wonder, why only a string typed option was used instead of an lambda.IFunction, the reason is, that this would introduce a circular dependency between lambda -> cloudwatch -> lambda and leads to build errors. Creating a new package for this tiny feature to work around the circular dependency would be too much of a good thing. Apart from that it could be unintuitive to find the custom widget in a package 'aws-cloudwatch-pattarns' if all other widgets come from 'aws-cloudwatch'. *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
This PR introcudes the CustomWidget for Cloudwatch Dashboards
fixes: #17579
based on the work of @dannyber
If you wonder, why only a string typed option was used instead of an lambda.IFunction, the reason is, that this would introduce a circular dependency between lambda -> cloudwatch -> lambda and leads to build errors.
Creating a new package for this tiny feature to work around the circular dependency would be too much of a good thing. Apart from that it could be unintuitive to find the custom widget in a package 'aws-cloudwatch-pattarns' if all other widgets come from 'aws-cloudwatch'.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license