Fix failing aws_ses_identity integration tests #39560
Conversation
Reduce boilerplate with yaml anchor
s-hertel
force-pushed
the
fix_aws_ses_identity_tests
branch
from
2a21e71
to
7033b15
Compare
ansibot
added
aws
bug
s-hertel
removed
the
needs_triage
|
cc @orthanc |
ansibot
added
the
stale_ci
There was a problem hiding this comment.
Edit: This should have been assoicated with this line: https://github.com/ansible/ansible/pull/39560/files#diff-ad7f4dd9f70573d8f8d2f2ece53c0080R384
I think it would be better to check for BounceTopic and ComplaintTopic in the module params rather than identity_notification.
I've noticed a lot of eventual consistentency type issues with these APIs, so while theoretically it should be OK to check identity notification here since we re-loaded it I think it would be better to just make sure the desired state topics and feedback forwarding are consistent.
Essentially checking that both of the following are truthy:
module.params.get('bouce_notifications')['topic']
module.params.get('complaint_notifications')['topic']
Both the params might be None though so probably need a need a none check.
ansibot
added
needs_revision
| @@ -409,6 +413,7 @@ def update_identity_notifications(connection, module): | |||
| for notification_type in ('Bounce', 'Complaint', 'Delivery'): | |||
| changed |= update_notification_topic(connection, module, identity, identity_notifications, notification_type) | |||
| changed |= update_notification_topic_headers(connection, module, identity, identity_notifications, notification_type) | |||
| identity_notifications = get_identity_notifications(connection, module, identity) | |||
There was a problem hiding this comment.
There should be no need to do this in the loop. If we want to keep the current approach of basing the feedback forwarding check on the reloaded identitiy notifications then we should move this outside the loop so that we only make one additional API call rather than 3.
The three topic settings won't affect each other so there should be no need to update between iterations of the loop.
But as I indicated in the other review comment, I think we're probably better checking the feedback forwarding state against the desired state of these topics rather than the reloaded identity_notifications since that way we avoid eventual consistency issues
|
@s-hertel I've made the changes I would like in my fork, be good if this commit could just be added to this PR as it's based on your changes: orthanc@7468ea7 Basically this is swapping to validating the desired feedback forwarding state against the desired topics state so there's no need to re-load the notification state from the API. I've also added a couple of additional integration tests for the "specified 1 topic" cases to make sure the logic remains correct. |
repeated API calls.
|
Thanks so much @orthanc! Sorry for taking a bit to get back to this. |
ansibot
removed
the
stale_ci
|
shipit |
ansibot
added
shipit
|
@s-hertel Will this be backported to stable-2.5? |
* Fix failing aws_ses_identity integration tests Reduce boilerplate with yaml anchor * remove unstable test alias * Update feedback forwarding check to use desired state rather than repeated API calls. (cherry picked from commit 571c183)
* Fix failing aws_ses_identity integration tests Reduce boilerplate with yaml anchor * remove unstable test alias * Update feedback forwarding check to use desired state rather than repeated API calls.
* Fix failing aws_ses_identity integration tests (#39560) * Fix failing aws_ses_identity integration tests Reduce boilerplate with yaml anchor * remove unstable test alias * Update feedback forwarding check to use desired state rather than repeated API calls. (cherry picked from commit 571c183) * changelog
* Fix failing aws_ses_identity integration tests Reduce boilerplate with yaml anchor * remove unstable test alias * Update feedback forwarding check to use desired state rather than repeated API calls.
* Fix failing aws_ses_identity integration tests Reduce boilerplate with yaml anchor * remove unstable test alias * Update feedback forwarding check to use desired state rather than repeated API calls.
Reduce boilerplate with yaml anchor
SUMMARY
Fixes #38713
At one point, an InvalidParameterValue error was raised in response to trying to turn off feedback forwarding if there are not bounce and complaint topics. Now this succeeds (using the AWS command line as well). An alternative would be to remove the additional safeguard code I added and remove the test that used to fail.
@orthanc Please review and I'll update the pull request if you want me to solve this differently.
ISSUE TYPE
COMPONENT NAME
lib/ansible/modules/cloud/amazon/aws_ses_identity.py
test/integration/targets/aws_ses_identity/tasks/main.yaml
ANSIBLE VERSION