Skip to content

fix (aws-cloudwatch): use dimensionsMap property instead of depresiated dimensions in Metric class(fixes the docs issue) #21647

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

Closed
wants to merge 3 commits into from

Conversation

ahzia
Copy link

@ahzia ahzia commented Aug 17, 2022


The Metric Class does not have the dimentionsMap property, instead, it includes the depreciated dimensions, So in the documentation, It is not showing dimentionsMap.
I added dimentionsMap into the class and documented the "dimensions" property as depreciated.

fixes #21618

All Submissions:

Adding new Unconventional Dependencies:

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

@gitpod-io
Copy link

gitpod-io bot commented Aug 17, 2022

Copy link
Contributor

@TheRealAmazonKendra TheRealAmazonKendra left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution! We will need some tests for this change.

Please make sure that your PR title confirms to the conventional commit standard (fix, feat, chore) and that it is written in a style that will reflect correctly in the change log (See Contributing Guide, Pull Requests)

Additionally, please make sure that your PR body describes the problem the PR is solving, and the design approach and alternatives considered. Explain why the PR solves the problem. A link to an issue is helpful, but does not replace an explanation of your thought process.

@ahzia ahzia changed the title AWS CDK: use dimensionsMap property instead of depresiated dimensions in metric docs fix (aws-cloudwatch): use dimensionsMap property instead of depresiated dimensions in metric docs Aug 19, 2022
@ahzia
Copy link
Author

ahzia commented Aug 19, 2022

@TheRealAmazonKendra Thanks for your review, I have made some updates to the PR name and description.

@mergify mergify bot dismissed TheRealAmazonKendra’s stale review August 19, 2022 12:59

Pull request has been modified.

@ahzia ahzia marked this pull request as ready for review August 19, 2022 13:00
@ahzia ahzia changed the title fix (aws-cloudwatch): use dimensionsMap property instead of depresiated dimensions in metric docs fix (aws-cloudwatch): use dimensionsMap property instead of depresiated dimensions in Metric class(fixes the docs issue) Aug 19, 2022
@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
  • Commit ID: a46d2d3
  • Result: FAILED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

Copy link
Contributor

@peterwoodworth peterwoodworth left a comment

Choose a reason for hiding this comment

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

We will want tests for this @ahzia,

In addition to this and the other comment I made, you have some issues that are throwing errors in our build that you'll need to fix

@aws-cdk/aws-cloudwatch: /codebuild/output/src931296454/src/github.com/aws/aws-cdk/packages/@aws-cdk/aws-cloudwatch/lib/metric.ts
@aws-cdk/aws-cloudwatch:   254:6   error  Trailing spaces not allowed  no-trailing-spaces
@aws-cdk/aws-cloudwatch:   255:31  error  Trailing spaces not allowed  no-trailing-spaces

public readonly dimensions?: DimensionHash;
/** Dimensions of this metric */
public readonly dimensionsMap?: DimensionsMap;
Copy link
Contributor

Choose a reason for hiding this comment

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

You're creating this new prop, but not doing anything with it yet so it will always be undefined. There are a few other places where we use this.dimensions that you'll need to account for with this new prop if we are to deprecate the old prop

@aws-cdk-automation
Copy link
Collaborator

This PR has been in the BUILD FAILING state for 3 weeks, and looks abandoned. To keep this PR from being closed, please continue work on it. If not, it will automatically be closed in a week.

@aws-cdk-automation
Copy link
Collaborator

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.

@aws-cdk-automation aws-cdk-automation added the closed-for-staleness This issue was automatically closed because it hadn't received any attention in a while. label Sep 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
closed-for-staleness This issue was automatically closed because it hadn't received any attention in a while. p2
Projects
None yet
Development

Successfully merging this pull request may close these issues.

AWS CDK: Rename dimensions metric property to dimensionsMap
4 participants