Skip to content

Conversation

kddejong
Copy link
Contributor

Issue #, if available:
Fix #766
Description of changes:

  • Sort Exclusive and OnlyOne json docs
  • Update OnlyOne for CloudWatch Alarm to require MetricName or Metrics
  • Update Exclusive for CloudWatch Alarm to have when Metrics are specified that Namespace, Dimensions, and Period are not defined

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@kddejong
Copy link
Contributor Author

When dropping Metrics, Statistics, and ExtendedStatistic you will get this error. MetricName must not be null (Service: AmazonCloudWatch; Status Code: 400; Error Code: ValidationError; ...) so Updated the definition as needed for OnlyOne.

From https://docs.aws.amazon.com/AmazonCloudWatch/latest/APIReference/API_PutMetricAlarm.html
there is also this.

If you use the Metrics parameter, you cannot include the MetricName, Dimensions, Period, Namespace, Statistic, or ExtendedStatistic parameters of PutMetricAlarm in the same operation. Instead, you retrieve the metrics you are using in your math expression as part of the Metrics array.

@codecov
Copy link

codecov bot commented Mar 28, 2019

Codecov Report

Merging #771 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #771   +/-   ##
=======================================
  Coverage   85.54%   85.54%           
=======================================
  Files         121      121           
  Lines        7084     7084           
  Branches     1702     1702           
=======================================
  Hits         6060     6060           
  Misses        638      638           
  Partials      386      386

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 89a5a5f...0b02786. Read the comment docs.

Copy link
Contributor

@cmmeyer cmmeyer left a comment

Choose a reason for hiding this comment

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

Seems reasonable. Do the test fixtures need to be modified to include these cases?

Copy link
Contributor

@cmmeyer cmmeyer left a comment

Choose a reason for hiding this comment

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

At the very least it looks like this impacts the number of expected failures in the unit tests:

AssertionError: Expected 57 failures, got 63 on test/fixtures/templates/quickstart/cis_benchmark.yaml

@kddejong kddejong merged commit 30c7193 into aws-cloudformation:master Mar 28, 2019
@kddejong kddejong deleted the Fix/766 branch March 28, 2019 22:26
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.

2 participants