feat: Set S3 registry user agent, document endpoint override - #6685
Draft
goanpeca wants to merge 1 commit into
Draft
feat: Set S3 registry user agent, document endpoint override#6685goanpeca wants to merge 1 commit into
goanpeca wants to merge 1 commit into
Conversation
Signed-off-by: Gonzalo Peña-Castellanos <[email protected]>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this PR does / why we need it:
S3RegistryStorealready readsFEAST_S3_ENDPOINT_URLand hands it to boto3, so the registry can live on an S3-compatible object store, but the variable is not mentioned anywhere in the registry reference docs. The repo already depends on it in practice (examples/python-helm-demo/minio.env, the Helm example's Kubernetes env patch, and the MinIO container intest_universal_registry.py), so this adds a short section todocs/reference/registries/s3.mdcovering the variable and the credentials boto3 resolves alongside it.It also sets the Feast user agent on the S3 registry's boto3 client. Feast already tags its BigQuery, Datastore, DynamoDB, Redshift and Athena clients with
get_user_agent(), and the S3 registry client was the remaining gap. This passes it asuser_agent_extrarather thanuser_agentso the Feast tag is appended to botocore's default string instead of replacing it, which keeps the botocore, OS and Python metadata intact:The tag reuses the existing
feast.utils.get_user_agent()helper, whose version already resolves from package metadata with an"unknown"fallback. No new config keys, no new dependencies, and no behavior change for existing users.Which issue(s) this PR fixes:
None.
Checks
git commit -s)Testing Strategy
Misc
ruff checkandruff format --checkare clean onsdk/python/feast/infra/registry/s3.py, andmypy feast/infra/registry/s3.pyreports nothing in that file.I ran
tests/unit/test_registry_string_config.py, the only unit file that referencesS3RegistryStore, andtest_s3_scheme_selects_s3_registry_storepasses. The other failures in that file come from a missing optionalredisextra in my environment and reproduce identically on a cleanmaster, so they are unrelated to this change.I also confirmed by hand that
S3RegistryStorestill honorsFEAST_S3_ENDPOINT_URLand that the resolved user agent keeps botocore's default prefix withfeast-dev/feast/<version>appended (the string quoted above). Happy to split the docs and client changes into separate PRs, or to switch touser_agent=for consistency with the other clients, if you'd prefer either.