Skip to content

docs(flyte-binary): clarify s3 endpoint for compatible stores - #7735

Merged
Sovietaced merged 1 commit into
flyteorg:mainfrom
goanpeca:docs/s3-compatible-endpoint-note
Aug 4, 2026
Merged

docs(flyte-binary): clarify s3 endpoint for compatible stores#7735
Sovietaced merged 1 commit into
flyteorg:mainfrom
goanpeca:docs/s3-compatible-endpoint-note

Conversation

@goanpeca

Copy link
Copy Markdown
Contributor

Why are the changes needed?

configuration.storage.providerConfig.s3.endpoint already drives the stow S3 endpoint config key (see charts/flyte-binary/templates/configmap.yaml), which is what lets a deployment point its metadata and user-data buckets at an S3-compatible object store instead of Amazon S3. The current one-line comment, "URL of S3-compatible service", does not say that the value stays empty for Amazon S3, and does not show what shape the value takes, so it is easy to guess wrong (bare hostname vs full URL) on a first install. The devbox chart already relies on this key for a non-AWS store (charts/flyte-devbox/values.yaml), so the behavior is established and this only documents it.

What changes were proposed in this pull request?

Two comment lines above endpoint in charts/flyte-binary/values.yaml, following the existing two-line v2Signing comment style a few lines above: leave it empty for Amazon S3, otherwise set the provider URL, with an example URL shape and a few S3-compatible stores named as examples.

No values, templates, or defaults change.

How was this patch tested?

No tests were added, since the change is comment-only. Two local checks:

  1. helm-docs --chart-search-root=charts/flyte-binary with helm-docs v1.8.0 installed via go install, matching .github/workflows/check-helm-docs.yml, regenerates charts/flyte-binary/README.md with no diff. These are plain comments, not helm-docs # -- comments, so the generated table is unaffected.
  2. values.yaml parsed with yaml.safe_load and compared against main: the resulting tree is identical, so rendered manifests are unchanged.

Labels

changed

Check all the applicable boxes

  • I updated the documentation accordingly.
  • All new and existing tests passed. (comment-only change, no test run; the helm-docs check was verified locally)
  • All commits are signed-off.

Signed-off-by: Gonzalo Peña-Castellanos <goanpeca@gmail.com>
Copilot AI review requested due to automatic review settings July 31, 2026 16:14

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Clarifies how to set configuration.storage.providerConfig.s3.endpoint in the flyte-binary Helm chart so users correctly configure S3-compatible object stores while leaving the value empty for Amazon S3.

Changes:

  • Expanded the inline comment above configuration.storage.providerConfig.s3.endpoint to specify it should be empty for Amazon S3.
  • Added examples of S3-compatible providers and an example endpoint URL shape.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@pingsutw pingsutw left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for updating it. Is it ready to review?

@goanpeca
goanpeca marked this pull request as ready for review August 4, 2026 20:40
@goanpeca

goanpeca commented Aug 4, 2026

Copy link
Copy Markdown
Contributor Author

Yes, ready for review, thanks for the ping @pingsutw. Marked it out of draft.

For context on the change: configuration.storage.providerConfig.s3.endpoint was already plumbed through to the stow S3 backend, but the inline comment in values.yaml did not say that leaving it empty targets Amazon S3 while setting it targets any store implementing the S3 API. This is a comment-only edit to that one key, no default changed. I verified helm-docs regenerates charts/flyte-binary/README.md with no diff, and that the parsed values.yaml is byte-identical apart from the comment.

@Sovietaced
Sovietaced merged commit f2f667e into flyteorg:main Aug 4, 2026
24 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants