Sns/v2 sms attribute operations #13255
Conversation
baermat
force-pushed
the
sns/v2-sms-attribute-operations
branch
from
a457285 to
9a6b3b5
Compare
baermat
left a comment
baermat
left a comment
There was a problem hiding this comment.
I marked some parts which I am not too sure about, I would love to hear some feedback
| def _set_sms_attribute_default(store: SnsStore) -> None: | ||
| # TODO: don't call this on every sms attribute crud api call | ||
| store.sms_attributes.setdefault("MonthlySpendLimit", "1") |
There was a problem hiding this comment.
I am not very happy with this, but I am not sure how to properly tackle this: When the provider is loaded, MonthlySpendLimit should be set to 1. This is saved per region however. which means I cannot really deal with this with a hook like "on_after_state_loaded". I could do it for every region, but then I am creating objects in stores for all regions, which might have side effects I am not aware of. The current solution shouldn't have a huge impact, it's just not pretty
There was a problem hiding this comment.
I think it's an acceptable solution for now, I don't think it will have have a huge cost, and it's pretty clear what it does 👍 I think this is good! nice pragmatic solution
| if is_aws_cloud(): | ||
| LOG.warning( | ||
| "Warning: this test will permanently set values for sms attributes in this region. \n Removing is not possible, only overwriting.\n Comment the assert if you wish to proceed" | ||
| ) | ||
| raise Exception("Check logs to enable this test against AWS") |
There was a problem hiding this comment.
This is also something I am not too sure about how to deal with it. The problem is thus:
Setting one of the allowed attributes sets it permanently in AWS. Afterwards, you can only override it, but not remove it (which alters get-sms-attributes calls that retrieve all attributes). I am not sure if we can simply ignore this, and if this very strict abort is what we want
There was a problem hiding this comment.
Agree, this is a tricky situation! I think this is an acceptable solution as well, and the error message and warning is good enough. I would however switch the markers.aws to manual_setup_required instead validated , to indicate that it might not be that easy to run against AWS. It's not "purely" manual setup required, but it might warn people a bit more that it is "a special case", what do you think?
tests/aws/services/sns/test_sns.py
Outdated
| @markers.aws.validated | ||
| def test_get_sms_attributes_from_unmodified_region( | ||
| self, aws_client_factory, secondary_region_name, snapshot | ||
| ): | ||
| # if the secondary region is already modified, you can use SECONDARY_TEST_AWS_PROFILE and similar | ||
| # variables to change it | ||
| sns_unmodified_region = aws_client_factory(region_name=secondary_region_name).sns | ||
| response = sns_unmodified_region.get_sms_attributes() | ||
| snapshot.match("get-sms-attributes_unmodified_region", response) | ||
|
|
There was a problem hiding this comment.
Same problem as before, this validates that all get_sms_attributes_call return at least MonthlySpendLimit = 1, even though you never set anything in this region. However, the person executing this test may have set an attribute at some point in time in that particular region
There was a problem hiding this comment.
Agreed, I would also set manual_setup_required, and might move the test just so that it sits right besides test_set_get_sms_attributes.
we should also maybe manually assert in this test that the value is set to 1, so that people would not inadvertently update the snapshot with some non-default values?
There was a problem hiding this comment.
👍 The manual assert might be a bit extra safe, but then again we do not want to rely on LS having correct behaviour to "correctly" cause a snapshot mismatch in such a case.
baermat
added
semver: minor
LocalStack Community integration with Pro 2 files ±0 2 suites ±0 59m 55s ⏱️ +17s For more details on these failures, see this check. Results for commit 847e34d. ± Comparison against base commit b4770eb. ♻️ This comment has been updated with latest results. |
bentsku
left a comment
bentsku
left a comment
There was a problem hiding this comment.
Nice work! I like that you went with a pragmatic solution for both the default values and the testing strategy, I think this is a good workaround around the issue.
Just have some comment about the validation and the testing markers; once this is addressed we're good to merge!
| def _set_sms_attribute_default(store: SnsStore) -> None: | ||
| # TODO: don't call this on every sms attribute crud api call | ||
| store.sms_attributes.setdefault("MonthlySpendLimit", "1") |
There was a problem hiding this comment.
I think it's an acceptable solution for now, I don't think it will have have a huge cost, and it's pretty clear what it does 👍 I think this is good! nice pragmatic solution
| default_send_id = attributes.get("DefaultSendID") | ||
| if default_send_id and not re.match(SMS_DEFAULT_SENDER_REGEX, default_send_id): | ||
| raise InvalidParameterException(f"{k} is not a valid attribute") | ||
| sms_type = attributes.get("DefaultSMSType") | ||
| if sms_type and sms_type not in SMS_TYPES: | ||
| raise InvalidParameterException("DefaultSMSType is invalid") |
There was a problem hiding this comment.
nit: I think this code should not be in the loop, as it directly accesses the attributes
| raise InvalidParameterException(f"{k} is not a valid attribute") | ||
| default_send_id = attributes.get("DefaultSendID") | ||
| if default_send_id and not re.match(SMS_DEFAULT_SENDER_REGEX, default_send_id): | ||
| raise InvalidParameterException(f"{k} is not a valid attribute") |
There was a problem hiding this comment.
is this the right error message? it seems unrelated to the k variable and only to DefaultSendID, so we might want to hardcode that value.
Right now if you were to set 2 attributes, and DefaultSendID would be the second one in the dictionary, you'd return the wrong k
| if is_aws_cloud(): | ||
| LOG.warning( | ||
| "Warning: this test will permanently set values for sms attributes in this region. \n Removing is not possible, only overwriting.\n Comment the assert if you wish to proceed" | ||
| ) | ||
| raise Exception("Check logs to enable this test against AWS") |
There was a problem hiding this comment.
Agree, this is a tricky situation! I think this is an acceptable solution as well, and the error message and warning is good enough. I would however switch the markers.aws to manual_setup_required instead validated , to indicate that it might not be that easy to run against AWS. It's not "purely" manual setup required, but it might warn people a bit more that it is "a special case", what do you think?
tests/aws/services/sns/test_sns.py
Outdated
| @markers.aws.validated | ||
| def test_get_sms_attributes_from_unmodified_region( | ||
| self, aws_client_factory, secondary_region_name, snapshot | ||
| ): | ||
| # if the secondary region is already modified, you can use SECONDARY_TEST_AWS_PROFILE and similar | ||
| # variables to change it | ||
| sns_unmodified_region = aws_client_factory(region_name=secondary_region_name).sns | ||
| response = sns_unmodified_region.get_sms_attributes() | ||
| snapshot.match("get-sms-attributes_unmodified_region", response) | ||
|
|
There was a problem hiding this comment.
Agreed, I would also set manual_setup_required, and might move the test just so that it sits right besides test_set_get_sms_attributes.
we should also maybe manually assert in this test that the value is set to 1, so that people would not inadvertently update the snapshot with some non-default values?
bentsku
left a comment
bentsku
left a comment
There was a problem hiding this comment.
LGTM! Thanks for addressing the comments, this looks good! Nice work 💯
baermat
force-pushed
the
sns/v2-sms-attribute-operations
branch
from
847e34d to
3e21dff
Compare
Motivation
Adds SMS attributes, with the following restrictions:
closes PNX-84
Changes