Require Valid Elasticache Engine Type #8539
Conversation
|
@bblommers Small contribution, ready for review |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #8539 +/- ##
=======================================
Coverage 92.63% 92.63%
=======================================
Files 1229 1229
Lines 106422 106433 +11
=======================================
+ Hits 98580 98591 +11
Misses 7842 7842
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
I think the heart of the issue is that regardless if the engine type parameter is Redis or redis, we want to store the value as lowercase. This changeset, as currently written, would raise an error if Redis was passed in, which doesn't match AWS behavior. Just converting the engine parameter to lowercase in responses.py:create_user would be the simplest way to address this.
|
|
||
| class ValidAuthModeKeys(str, Enum): | ||
| TYPE = "Type" | ||
| PASSWORD = "Passwords" |
There was a problem hiding this comment.
I get that you were probably just following the existing convention, but I don't think we gain anything by using Enum here. An array constant, e.g. VALID_ENGINE_TYPES = ["redis", "valkey"], would be simpler, allowing you to compare directly without the separate step of converting the enum values into an array.
| == 'Unknown parameter for Engine: "invalidengine", must be one of: redis, valkey' | ||
| ) | ||
|
|
||
|
|
There was a problem hiding this comment.
Checking that the returned error code is "InvalidParameterValue" should suffice. We don't want to assert on something that would break if another engine type is added in the future. If you want to be more thorough with your asserts while keeping your test future-proof, you can just check that "Unknown parameter for Engine" is in the message.
| @mock_aws | ||
| def test_create_user_with_password(): | ||
| client = boto3.client("elasticache", region_name="ap-southeast-1") | ||
| user_id = "user1" | ||
| resp = client.create_user( | ||
| UserId=user_id, | ||
| UserName="User1", | ||
| Engine="Redis", | ||
| Engine="redis", |
There was a problem hiding this comment.
AWS allows for Redis or redis to be passed as the engine type, so let's leave all create_user calls as they are and just assert (as you have) that it's redis in the responses.
Resolves issue #8520