Make "unit" parameter optional and add support for check mode #470
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
boto3 documentation explicitly suggests omitting this parameter: https://boto3.amazonaws.com/v1/documentation/api/latest/reference/services/cloudwatch.html#CloudWatch.Client.put_metric_alarm Creating an alarm without the "unit" parameter specified fails: ``` - community.aws.ec2_metric_alarm: name: My alarm description: My description namespace: AWS/CertificateManager metric: DaysToExpiry statistic: Average comparison: LessThanOrEqualToThreshold threshold: 45 period: 86400 evaluation_periods: 1 dimensions: CertificateArn: "arn:aws:acm:us-east-1:123412341234:certificate/example" alarm_actions: - arn:aws:sns:us-east-1:123412341234:my-sns-topic ok_actions: - arn:aws:sns:us-east-1:123412341234:my-sns-topic treat_missing_data: ignore state: present ``` with the following error: ``` Invalid type for parameter Unit, value: None, type: <class 'NoneType'>, valid types: <class 'str'> ``` Apparently specifying `unit: None` in the example above is not the same as omitting the unit - it causes the alarm to be in "Insufficient data" state.
ansibullbot
added
community_review
feature
tremble
requested changes
Mar 11, 2021
There was a problem hiding this comment.
Hi @ichekaldin thanks for your PR.
We have some integration tests (tests/integration/targets/ec2_metric_alarm) for this module, it would be good if you could expand these to include testing your latest changes.
While the tests currently run a loop of
- make-change
- describe-alarms
It would be better (if you're willing) to switch this over to a loop of:
- make-change (check_mode=True) - should return Changed
- make-change (check_mode=False) - should return Changed
- make-change (check_mode=True) - should return Not Changed
- make-change (check_mode=False) - should return Not Changed
for each test
This 4-step loop helps test both idempotency and check_mode
ansibullbot
added
needs_revision
tremble
requested changes
Mar 11, 2021
|
The check mode tests seem to be failing: https://app.shippable.com/github/ansible-collections/community.aws/runs/1818/37/tests |
Alarm is not actuall created in check mode, and therefore `describe_alarms` returns an empty list.
|
@tremble It took me a few iterations to get the integrations tests working. It looks like the PR is now failing the ansible/check test. I see this in the output: I'm not 100% sure how to move this forward as neither |
ansibullbot
added
community_review
and removed
needs_revision
|
You're looking at the correct error. There was a recent update to ansible-test's sanity tests which picked up some issues now addressed by #471 given that Shippable's passing. I wouldn't worry about Zuul right now (we still have some work to do getting things passing in Zuul) |
tremble
approved these changes
Mar 12, 2021
tremble
reviewed
Mar 12, 2021
danquixote
pushed a commit
to danquixote/community.aws
that referenced
this pull request
May 16, 2021
…e-collections#470) * Make "unit" parameter optional and add support for check mode boto3 documentation explicitly suggests omitting this parameter: https://boto3.amazonaws.com/v1/documentation/api/latest/reference/services/cloudwatch.html#CloudWatch.Client.put_metric_alarm Creating an alarm without the "unit" parameter specified fails: ``` - community.aws.ec2_metric_alarm: name: My alarm description: My description namespace: AWS/CertificateManager metric: DaysToExpiry statistic: Average comparison: LessThanOrEqualToThreshold threshold: 45 period: 86400 evaluation_periods: 1 dimensions: CertificateArn: "arn:aws:acm:us-east-1:123412341234:certificate/example" alarm_actions: - arn:aws:sns:us-east-1:123412341234:my-sns-topic ok_actions: - arn:aws:sns:us-east-1:123412341234:my-sns-topic treat_missing_data: ignore state: present ``` with the following error: ``` Invalid type for parameter Unit, value: None, type: <class 'NoneType'>, valid types: <class 'str'> ``` Apparently specifying `unit: None` in the example above is not the same as omitting the unit - it causes the alarm to be in "Insufficient data" state. * Fix module output for tests * Add tests for idempotency and check mode * Fix an error when the module creates a new alarm in check mode Alarm is not actuall created in check mode, and therefore `describe_alarms` returns an empty list. * Add tests for alarm creation with no unit attribute specified * Fix typo - MetricAlarms vs MetricsAlarms * Fix variable name - alarm_info_query_check vs alarm_info_check * Fix variable names in tests * Fix tests by ensuring that alarm doesn't exist before we begin * Fix variable name * Fix assertion * Ensure check mode is enabled when it is supposed to be * Enable check mode for alarm deletion * Fix variable name - alarm_info_no_unit vs alarm_info * Fix the test of creating the alarm without unit attribute * Fix variable name - alarm_info_query_no_unit vs alarm_info * Update changelogs/fragments/470-ec2_metric_alarm-unit-optional.yml * Update changelogs/fragments/470-ec2_metric_alarm-unit-optional.yml Co-authored-by: Mark Chappell <mchappel@redhat.com>
alinabuzachis
pushed a commit
to alinabuzachis/community.aws
that referenced
this pull request
Jul 19, 2021
…e-collections#470) * Make "unit" parameter optional and add support for check mode boto3 documentation explicitly suggests omitting this parameter: https://boto3.amazonaws.com/v1/documentation/api/latest/reference/services/cloudwatch.html#CloudWatch.Client.put_metric_alarm Creating an alarm without the "unit" parameter specified fails: ``` - community.aws.ec2_metric_alarm: name: My alarm description: My description namespace: AWS/CertificateManager metric: DaysToExpiry statistic: Average comparison: LessThanOrEqualToThreshold threshold: 45 period: 86400 evaluation_periods: 1 dimensions: CertificateArn: "arn:aws:acm:us-east-1:123412341234:certificate/example" alarm_actions: - arn:aws:sns:us-east-1:123412341234:my-sns-topic ok_actions: - arn:aws:sns:us-east-1:123412341234:my-sns-topic treat_missing_data: ignore state: present ``` with the following error: ``` Invalid type for parameter Unit, value: None, type: <class 'NoneType'>, valid types: <class 'str'> ``` Apparently specifying `unit: None` in the example above is not the same as omitting the unit - it causes the alarm to be in "Insufficient data" state. * Fix module output for tests * Add tests for idempotency and check mode * Fix an error when the module creates a new alarm in check mode Alarm is not actuall created in check mode, and therefore `describe_alarms` returns an empty list. * Add tests for alarm creation with no unit attribute specified * Fix typo - MetricAlarms vs MetricsAlarms * Fix variable name - alarm_info_query_check vs alarm_info_check * Fix variable names in tests * Fix tests by ensuring that alarm doesn't exist before we begin * Fix variable name * Fix assertion * Ensure check mode is enabled when it is supposed to be * Enable check mode for alarm deletion * Fix variable name - alarm_info_no_unit vs alarm_info * Fix the test of creating the alarm without unit attribute * Fix variable name - alarm_info_query_no_unit vs alarm_info * Update changelogs/fragments/470-ec2_metric_alarm-unit-optional.yml * Update changelogs/fragments/470-ec2_metric_alarm-unit-optional.yml Co-authored-by: Mark Chappell <mchappel@redhat.com>
alinabuzachis
pushed a commit
to alinabuzachis/community.aws
that referenced
this pull request
Jul 19, 2021
…e-collections#470) * Make "unit" parameter optional and add support for check mode boto3 documentation explicitly suggests omitting this parameter: https://boto3.amazonaws.com/v1/documentation/api/latest/reference/services/cloudwatch.html#CloudWatch.Client.put_metric_alarm Creating an alarm without the "unit" parameter specified fails: ``` - community.aws.ec2_metric_alarm: name: My alarm description: My description namespace: AWS/CertificateManager metric: DaysToExpiry statistic: Average comparison: LessThanOrEqualToThreshold threshold: 45 period: 86400 evaluation_periods: 1 dimensions: CertificateArn: "arn:aws:acm:us-east-1:123412341234:certificate/example" alarm_actions: - arn:aws:sns:us-east-1:123412341234:my-sns-topic ok_actions: - arn:aws:sns:us-east-1:123412341234:my-sns-topic treat_missing_data: ignore state: present ``` with the following error: ``` Invalid type for parameter Unit, value: None, type: <class 'NoneType'>, valid types: <class 'str'> ``` Apparently specifying `unit: None` in the example above is not the same as omitting the unit - it causes the alarm to be in "Insufficient data" state. * Fix module output for tests * Add tests for idempotency and check mode * Fix an error when the module creates a new alarm in check mode Alarm is not actuall created in check mode, and therefore `describe_alarms` returns an empty list. * Add tests for alarm creation with no unit attribute specified * Fix typo - MetricAlarms vs MetricsAlarms * Fix variable name - alarm_info_query_check vs alarm_info_check * Fix variable names in tests * Fix tests by ensuring that alarm doesn't exist before we begin * Fix variable name * Fix assertion * Ensure check mode is enabled when it is supposed to be * Enable check mode for alarm deletion * Fix variable name - alarm_info_no_unit vs alarm_info * Fix the test of creating the alarm without unit attribute * Fix variable name - alarm_info_query_no_unit vs alarm_info * Update changelogs/fragments/470-ec2_metric_alarm-unit-optional.yml * Update changelogs/fragments/470-ec2_metric_alarm-unit-optional.yml Co-authored-by: Mark Chappell <mchappel@redhat.com>
danielcotton
pushed a commit
to danielcotton/community.aws
that referenced
this pull request
Nov 23, 2021
…e-collections#470) * Make "unit" parameter optional and add support for check mode boto3 documentation explicitly suggests omitting this parameter: https://boto3.amazonaws.com/v1/documentation/api/latest/reference/services/cloudwatch.html#CloudWatch.Client.put_metric_alarm Creating an alarm without the "unit" parameter specified fails: ``` - community.aws.ec2_metric_alarm: name: My alarm description: My description namespace: AWS/CertificateManager metric: DaysToExpiry statistic: Average comparison: LessThanOrEqualToThreshold threshold: 45 period: 86400 evaluation_periods: 1 dimensions: CertificateArn: "arn:aws:acm:us-east-1:123412341234:certificate/example" alarm_actions: - arn:aws:sns:us-east-1:123412341234:my-sns-topic ok_actions: - arn:aws:sns:us-east-1:123412341234:my-sns-topic treat_missing_data: ignore state: present ``` with the following error: ``` Invalid type for parameter Unit, value: None, type: <class 'NoneType'>, valid types: <class 'str'> ``` Apparently specifying `unit: None` in the example above is not the same as omitting the unit - it causes the alarm to be in "Insufficient data" state. * Fix module output for tests * Add tests for idempotency and check mode * Fix an error when the module creates a new alarm in check mode Alarm is not actuall created in check mode, and therefore `describe_alarms` returns an empty list. * Add tests for alarm creation with no unit attribute specified * Fix typo - MetricAlarms vs MetricsAlarms * Fix variable name - alarm_info_query_check vs alarm_info_check * Fix variable names in tests * Fix tests by ensuring that alarm doesn't exist before we begin * Fix variable name * Fix assertion * Ensure check mode is enabled when it is supposed to be * Enable check mode for alarm deletion * Fix variable name - alarm_info_no_unit vs alarm_info * Fix the test of creating the alarm without unit attribute * Fix variable name - alarm_info_query_no_unit vs alarm_info * Update changelogs/fragments/470-ec2_metric_alarm-unit-optional.yml * Update changelogs/fragments/470-ec2_metric_alarm-unit-optional.yml Co-authored-by: Mark Chappell <mchappel@redhat.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
SUMMARY
boto3 documentation explicitly suggests omitting this parameter.
Creating an alarm without the "unit" parameter specified fails:
with the following error:
Apparently specifying
unit: Nonein the example above is not the same as omitting the unit -it causes the alarm to be in "Insufficient data" state.
ISSUE TYPE
COMPONENT NAME
ec2_metric_alarm
ADDITIONAL INFORMATION