bartek commented on PR #3439: URL: https://github.com/apache/tika/pull/3439#issuecomment-6446000906
@tballison Thanks! For context, we had forked apache/tika back when tika-pipes was its own project (via @nddipiazza's effort to support work we were doing there) and added the `identity` credentials provider to support our deployment needs. Regarding your machine's feedback, I pushed 3 fixup commits addressing these points: 1. I updated the s3.adoc. I'm not a bug infra guy so I used a machine to help me on this one, using what we've deployed and had success with as context 2. I fixed a small concatenation issue in S3PipesIterator.java 3. On the repeated 2 line comment. None of the other cases have a comment so it's likely best to match that until a documentation sweep is preferred. On items like: > 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. I suppose it's your call? A machine could easily add a helper and refactor, but want to keep the PR scope small. And: > - No CHANGES.txt entry. Happy to amend if this is the right PR for it. And finally: > 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. I feel `identity` matches the succinct naming terminology already present in these fetchers. -- 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]
