tballison commented on PR #3439:
URL: https://github.com/apache/tika/pull/3439#issuecomment-6445588578
Thank you for opening this.
Let me know what you think of my robot's feedback:
```
Worth raising:
- Option name: identity is vague. AWS calls this "web identity"
(AssumeRoleWithWebIdentity, AWS_WEB_IDENTITY_TOKEN_FILE), and the existing
values use snake_case (key_secret). web_identity fits both, and this name is
permanent config surface.
- EKS Pod Identity still won't work: it's now the AWS-recommended EKS
mechanism, and it uses container credentials, not web identity. instance
doesn't help, because it only queries the EC2 instance metadata service, so it
never sees ECS task roles or Pod Identity. Either add a default option (the
SDK's standard chain, which covers both) or say explicitly that Pod Identity is
out of scope.
- Existing docs are wrong:
- s3.adoc says instance covers "ECS task role, etc.", which is false for
the reason above.
- It also says the fetcher and iterator don't validate the option, but
they throw when the component is created.
The PR edits the lines next to both and keeps both claims.
- Fourth copy of the provider switch: it extracts the fetcher's switch
into a method but leaves the emitter and iterator copies in place. The value
list and error message now live in four places. One shared helper would fix
that.
- Tests check mappings, not behavior: they assert which provider class is
returned, plus Class.forName for the STS factory.
That check does catch the important regression (dropping sts), since sts
isn't on the classpath any other way. Nothing shows the token is actually
exchanged and the result used to sign requests. That's acceptable, but my
S3CredentialsTest does exactly that with a local mock STS and S3 if they want
it.
Minor:
- No CHANGES.txt entry.
- The same two-line comment is pasted three times.
- The iterator's error message still has the leftover "…and " + "must be…"
string concatenation.
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
