Repository navigation
Port IAM authentication to Java - #10
Conversation
mwaisaida-13
left a comment
There was a problem hiding this comment.
Solid port, the same known-answer vectors as Python/Node, and a complete test suite.
A few small changes before approving: centralize the supported Java versions like we do with pyproject.toml; use the Java builder parameter names in error messages; add a Lettuce RedisCredentialsProvider example alongside the RESP one; move PublicApiTest into a java/integration-tests/ dir so examples/ stays for documentation; order the README sections like python/README.md and add the engine floor. Thank you!
| fail-fast: false | ||
| matrix: | ||
| os: [ubuntu-latest] | ||
| java-version: ["8", "11", "17", "21"] |
There was a problem hiding this comment.
Could we have the supported versions centralized? that way it's simpler to add more versions. For python we store and extract them from the .toml
| .region(region) | ||
| .build(); | ||
|
|
||
| SSLSocketFactory socketFactory = (SSLSocketFactory) SSLSocketFactory.getDefault(); |
There was a problem hiding this comment.
Nice! a client-independent example showing the wire exchange is useful for custom clients and proxies. Could we also add a Lettuce RedisCredentialsProvider example alongside it, mirroring python/examples/connect_valkey.py, so typical client users see the short integration path?
There was a problem hiding this comment.
Added ConnectWithLettuce
| @@ -0,0 +1,27 @@ | |||
| /* | |||
There was a problem hiding this comment.
Not sure if we really need this test since the examples are mainly for documentation purposes, what do you think?
| @@ -0,0 +1,26 @@ | |||
| /* | |||
| * Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. | |||
There was a problem hiding this comment.
nit: This is really a consumer-side check against the installed jar, not an example. Could it move to its own java/integration-tests/ dir?
| Target target = resolveTarget(builder.serverlessCacheName, builder.replicationGroupId); | ||
| this.cacheName = target.cacheName; | ||
| this.serverless = target.serverless; | ||
| this.userId = validateAndNormalize("user_id", builder.userId, USER_ID_PATTERN); |
There was a problem hiding this comment.
nit: These messages name the Python parameters ('serverless_cache_name', 'replication_group_id', 'user_id'), but here we use serverlessCacheName, replicationGroupId, userId. Please use the Java names so the error points at something the caller can easily find.
| @@ -0,0 +1,101 @@ | |||
| # Developer Toolkit for Amazon ElastiCache (Java) | |||
There was a problem hiding this comment.
Could you order the sections like python/README.md (Installation → what you need → Generate a token → Reconnecting clients → Security → License) so the three READMEs read the same, and add the engine floor (Valkey 7.2+ / Redis OSS 7.0+) to the prerequisites?
There was a problem hiding this comment.
Reordered the README to Installation → What you need → Generate a token → Reconnecting clients → Security → License, and added both engine minimums.
| `credentialsProvider(...)`. | ||
|
|
||
| Missing credentials are reported when a token is requested. Expired or otherwise | ||
| invalid credentials cannot be detected during local signing: token generation can |
There was a problem hiding this comment.
This note is great, we should copy it to the other packages
|
Hey @alexr-bq, could you have a look at the merge conflicts? |
071b86e to
5fd9a6a
Compare
|
@mwaisaida-13 resolved |
Summary
Verification