Add pushing helm action #74
Conversation
PE-7046: Deploy Trino MCP Server
Pe 7046 fix workflow
WalkthroughA new GitHub Actions workflow for publishing Helm charts to the GitHub Container Registry was added. The release workflow was updated to use a lowercase repository name for Docker image caching and renamed for clarity. The Dockerfile was modified to use Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant MCP_Server
participant Okta_Server
Client->>MCP_Server: Request resource without token
MCP_Server->>Client: 401 Unauthorized with OAuth URL
Client->>MCP_Server: Request OAuth URL
MCP_Server->>Client: Redirect to Okta OAuth URL
Client->>Okta_Server: OAuth authorization request
Okta_Server->>MCP_Server: Callback with authorization code
MCP_Server->>Client: Proxy OAuth callback response
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
Note 🔌 MCP (Model Context Protocol) integration is now available in Early Access!Pro users can now connect to remote MCP servers under the Integrations page to get reviews and chat conversations that understand additional development context. ✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
Dockerfile (1)
30-55: Final stage should switch toscratchinstead ofalpine:latest.Internal guideline for every Dockerfile in this repo: “The Docker build must use a multi-stage build with a scratch base image for minimal size.”
Keepingalpine:latest(plusca-certificates) increases the image by ~6-7 MB and violates that rule.-FROM --platform=$TARGETPLATFORM alpine:latest -RUN apk --no-cache add ca-certificates +FROM --platform=$TARGETPLATFORM scratch +# Copy minimal cert bundle produced in builder stage (adds <300 kB) +COPY --from=builder /etc/ssl/certs/ca-certificates.crt /etc/ssl/certs/After switching to
scratch, ensure the binary is built withCGO_ENABLED=0(already done) and no other shared libs are required.
🧹 Nitpick comments (1)
.github/workflows/release.yml (1)
89-92: Environment variable created via$GITHUB_ENVcannot be used in expressions of the same step.Here you append
IMAGE_NAMEto$GITHUB_ENV, which only becomes available to later steps.No problem now (you use it later), but consider defining it once in the
env:block of the job to avoid an extra step:env: IMAGE_NAME: ${{ github.repository && toLower(github.repository) }}This keeps the job log cleaner and prevents accidental use inside the same step.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/push-helm-chart.yml(1 hunks).github/workflows/release.yml(3 hunks)Dockerfile(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
Dockerfile
📄 CodeRabbit Inference Engine (CLAUDE.md)
The Docker build must use a multi-stage build with a scratch base image for minimal size.
Files:
Dockerfile
🧠 Learnings (9)
📓 Common learnings
Learnt from: CR
PR: tuannvm/mcp-trino#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-25T23:51:10.585Z
Learning: Applies to .github/workflows/build.yml : Docker images must be published to GHCR as part of the build/release process.
Learnt from: CR
PR: tuannvm/mcp-trino#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-25T23:51:10.585Z
Learning: Applies to .github/workflows/build.yml : Multi-platform builds must be performed via GoReleaser as part of the build/release process.
Learnt from: CR
PR: tuannvm/mcp-trino#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-25T23:51:10.585Z
Learning: Applies to .github/workflows/build.yml : Automated releases must be triggered on main branch pushes in the CI pipeline.
Learnt from: CR
PR: tuannvm/mcp-trino#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-25T23:51:10.585Z
Learning: Applies to .github/workflows/build.yml : Go mod tidy must be run and verified as part of the CI pipeline.
📚 Learning: applies to dockerfile : the docker build must use a multi-stage build with a scratch base image for ...
Learnt from: CR
PR: tuannvm/mcp-trino#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-25T23:51:10.585Z
Learning: Applies to Dockerfile : The Docker build must use a multi-stage build with a scratch base image for minimal size.
Applied to files:
Dockerfile.github/workflows/release.yml
📚 Learning: applies to .github/workflows/build.yml : docker images must be published to ghcr as part of the buil...
Learnt from: CR
PR: tuannvm/mcp-trino#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-25T23:51:10.585Z
Learning: Applies to .github/workflows/build.yml : Docker images must be published to GHCR as part of the build/release process.
Applied to files:
.github/workflows/release.yml.github/workflows/push-helm-chart.yml
📚 Learning: applies to .github/workflows/build.yml : multi-platform builds must be performed via goreleaser as p...
Learnt from: CR
PR: tuannvm/mcp-trino#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-25T23:51:10.585Z
Learning: Applies to .github/workflows/build.yml : Multi-platform builds must be performed via GoReleaser as part of the build/release process.
Applied to files:
.github/workflows/release.yml.github/workflows/push-helm-chart.yml
📚 Learning: applies to .github/workflows/build.yml : automated releases must be triggered on main branch pushes ...
Learnt from: CR
PR: tuannvm/mcp-trino#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-25T23:51:10.585Z
Learning: Applies to .github/workflows/build.yml : Automated releases must be triggered on main branch pushes in the CI pipeline.
Applied to files:
.github/workflows/release.yml.github/workflows/push-helm-chart.yml
📚 Learning: applies to .github/workflows/build.yml : slsa provenance generation must be included for supply chai...
Learnt from: CR
PR: tuannvm/mcp-trino#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-25T23:51:10.585Z
Learning: Applies to .github/workflows/build.yml : SLSA provenance generation must be included for supply chain security in the CI pipeline.
Applied to files:
.github/workflows/release.yml
📚 Learning: applies to .github/workflows/build.yml : dependency verification must be performed in the ci pipelin...
Learnt from: CR
PR: tuannvm/mcp-trino#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-25T23:51:10.585Z
Learning: Applies to .github/workflows/build.yml : Dependency verification must be performed in the CI pipeline.
Applied to files:
.github/workflows/release.yml
📚 Learning: applies to .github/workflows/build.yml : sbom generation with spdx format must be included in the ci...
Learnt from: CR
PR: tuannvm/mcp-trino#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-25T23:51:10.585Z
Learning: Applies to .github/workflows/build.yml : SBOM generation with SPDX format must be included in the CI pipeline.
Applied to files:
.github/workflows/release.yml
📚 Learning: applies to .github/workflows/build.yml : code coverage must be collected with atomic mode and upload...
Learnt from: CR
PR: tuannvm/mcp-trino#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-25T23:51:10.585Z
Learning: Applies to .github/workflows/build.yml : Code coverage must be collected with atomic mode and uploaded to Codecov in the CI pipeline.
Applied to files:
.github/workflows/release.yml
🔇 Additional comments (2)
Dockerfile (1)
35-38: Change to/applooks good but remember to update health-check paths in Helm chart.Moving the working directory from
/rootto/appis fine.
Double-check any readiness/liveness probes ordocker execinstructions that may still assume/root..github/workflows/release.yml (1)
102-108: Confirm that${{ env.IMAGE_NAME }}is populated at this point.Because
IMAGE_NAMEis injected by the previous step, it should be available here, but any typo (step skipped, conditional false) will break cache references and the build will silently fall back to the remote layer cache.Please re-run the workflow once to verify that:
Buildx build context ... INFO Using cache mount: ghcr.io/<repo-lower>:cacheappears in the log.
| - name: Set up OCI Helper | ||
| run: | | ||
| echo "Installing Helm plugins..." | ||
| helm plugin install https://github.com/chartmuseum/helm-push || true | ||
|
|
There was a problem hiding this comment.
Chartmuseum plugin is unnecessary and may fail for OCI without HELM_EXPERIMENTAL_OCI.
Helm ≥ 3.8 includes native OCI push support via
helm chart push … – the extra plugin adds complexity and still needs:
export HELM_EXPERIMENTAL_OCI=1Proposed simplification:
-helm plugin install https://github.com/chartmuseum/helm-push || true
+echo "Using built-in OCI push (no plugin needed)"and later replace helm push with helm chart push.
🤖 Prompt for AI Agents
In .github/workflows/push-helm-chart.yml around lines 31 to 35, the current
setup installs the Chartmuseum Helm plugin which is unnecessary for Helm
versions 3.8 and above because they support native OCI push commands. Remove the
plugin installation step and update subsequent commands to use `helm chart push`
instead of `helm push`. Also, ensure that the environment variable
`HELM_EXPERIMENTAL_OCI` is not required or set, simplifying the workflow and
avoiding potential failures.
| - name: Push Helm Chart to GHCR | ||
| run: | | ||
| PACKAGE_NAME=$(ls .cr-release-packages/mcp-trino-*.tgz) | ||
| PACKAGE_VERSION=$(basename $PACKAGE_NAME | sed 's/mcp-trino-\(.*\)\.tgz/\1/') | ||
| echo "Publishing Helm chart $PACKAGE_NAME (version $PACKAGE_VERSION) to GHCR..." | ||
|
|
||
| # Push using Helm OCI | ||
| echo "Pushing to OCI registry..." | ||
| helm push $PACKAGE_NAME oci://ghcr.io/${{ env.REPO_LOWER }}/charts | ||
|
|
||
| echo "Helm chart published to ghcr.io/${{ env.REPO_LOWER }}/charts/mcp-trino:$PACKAGE_VERSION" |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Switch to native helm chart push and add a registry login to guarantee auth.
-helm push $PACKAGE_NAME oci://ghcr.io/${{ env.REPO_LOWER }}/charts
+helm registry login ghcr.io -u "${{ github.actor }}" -p "${{ secrets.GITHUB_TOKEN }}"
+helm chart push $PACKAGE_NAME oci://ghcr.io/${{ env.REPO_LOWER }}/chartsThis removes the experimental plugin dependency and works with current Helm releases.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - name: Push Helm Chart to GHCR | |
| run: | | |
| PACKAGE_NAME=$(ls .cr-release-packages/mcp-trino-*.tgz) | |
| PACKAGE_VERSION=$(basename $PACKAGE_NAME | sed 's/mcp-trino-\(.*\)\.tgz/\1/') | |
| echo "Publishing Helm chart $PACKAGE_NAME (version $PACKAGE_VERSION) to GHCR..." | |
| # Push using Helm OCI | |
| echo "Pushing to OCI registry..." | |
| helm push $PACKAGE_NAME oci://ghcr.io/${{ env.REPO_LOWER }}/charts | |
| echo "Helm chart published to ghcr.io/${{ env.REPO_LOWER }}/charts/mcp-trino:$PACKAGE_VERSION" | |
| - name: Push Helm Chart to GHCR | |
| run: | | |
| PACKAGE_NAME=$(ls .cr-release-packages/mcp-trino-*.tgz) | |
| PACKAGE_VERSION=$(basename $PACKAGE_NAME | sed 's/mcp-trino-\(.*\)\.tgz/\1/') | |
| echo "Publishing Helm chart $PACKAGE_NAME (version $PACKAGE_VERSION) to GHCR..." | |
| # Push using Helm OCI | |
| echo "Pushing to OCI registry..." | |
| helm registry login ghcr.io -u "${{ github.actor }}" -p "${{ secrets.GITHUB_TOKEN }}" | |
| helm chart push $PACKAGE_NAME oci://ghcr.io/${{ env.REPO_LOWER }}/charts | |
| echo "Helm chart published to ghcr.io/${{ env.REPO_LOWER }}/charts/mcp-trino:$PACKAGE_VERSION" |
🤖 Prompt for AI Agents
In .github/workflows/push-helm-chart.yml around lines 61 to 71, replace the
current helm push command that uses the experimental plugin with the native
'helm chart push' command. Additionally, add a step before pushing to perform a
'helm registry login' to GHCR using appropriate credentials to ensure
authentication. This change removes the dependency on the experimental plugin
and aligns with current Helm release capabilities.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
charts/mcp-trino/values.yaml (1)
32-36: Keep ALB example comments at the same indentation level for clarityThe ALB-related examples are indented two spaces deeper than the surrounding keys, which visually implies they belong inside the empty
annotationsmap. Flattening them avoids that confusion and keeps the file consistent.- # annotations for ALB ingress controller - # kubernetes.io/ingress.class: alb - # alb.ingress.kubernetes.io/scheme: internal - # alb.ingress.kubernetes.io/target-type: ip + # Example ALB ingress annotations: + # kubernetes.io/ingress.class: alb + # alb.ingress.kubernetes.io/scheme: internal + # alb.ingress.kubernetes.io/target-type: ip
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
charts/mcp-trino/values.yaml(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
PR: tuannvm/mcp-trino#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-25T23:51:10.585Z
Learning: Applies to .github/workflows/build.yml : Docker images must be published to GHCR as part of the build/release process.
Learnt from: CR
PR: tuannvm/mcp-trino#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-25T23:51:10.585Z
Learning: Applies to .github/workflows/build.yml : Multi-platform builds must be performed via GoReleaser as part of the build/release process.
Learnt from: CR
PR: tuannvm/mcp-trino#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-25T23:51:10.585Z
Learning: Applies to .github/workflows/build.yml : Automated releases must be triggered on main branch pushes in the CI pipeline.
🔇 Additional comments (1)
charts/mcp-trino/values.yaml (1)
32-36: Confirm the chart version was bumpedThe push-to-OCI workflow will publish the version declared in
Chart.yaml. If that version was not incremented, you’ll overwrite the existing artifact in GHCR and break any consumers relying on immutable tags. Please ensurecharts/mcp-trino/Chart.yamlhas itsversion(and, if applicable,appVersion) bumped in this PR.
* PE-7208: Support hostname in MCP URL Signed-off-by: Yang Wang <wyangsun@163.com> * Update oauth.md Signed-off-by: Yang Wang <wyangsun@163.com> * Update oauth.md Signed-off-by: Yang Wang <wyangsun@163.com> --------- Signed-off-by: Yang Wang <wyangsun@163.com>
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
README.md(2 hunks)docs/oauth.md(1 hunks)internal/mcp/handlers.go(6 hunks)internal/mcp/server.go(11 hunks)internal/oauth/config.go(1 hunks)internal/oauth/handlers.go(21 hunks)internal/oauth/metadata.go(7 hunks)
✅ Files skipped from review due to trivial changes (5)
- internal/oauth/config.go
- internal/mcp/handlers.go
- docs/oauth.md
- internal/mcp/server.go
- internal/oauth/handlers.go
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: CR
PR: tuannvm/mcp-trino#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-25T23:51:10.585Z
Learning: Applies to .github/workflows/build.yml : Docker images must be published to GHCR as part of the build/release process.
Learnt from: CR
PR: tuannvm/mcp-trino#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-25T23:51:10.585Z
Learning: Distribution must include GitHub Releases, GHCR, and Homebrew tap (tuannvm/mcp).
📚 Learning: 2025-07-25T23:51:10.585Z
Learnt from: CR
PR: tuannvm/mcp-trino#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-25T23:51:10.585Z
Learning: Applies to internal/handlers/trino_handlers.go : All MCP tool endpoints must handle parameter validation and error handling consistently.
Applied to files:
internal/oauth/metadata.goREADME.md
📚 Learning: 2025-07-25T23:51:10.585Z
Learnt from: CR
PR: tuannvm/mcp-trino#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-25T23:51:10.585Z
Learning: Applies to internal/handlers/trino_handlers.go : All MCP tool endpoints (execute_query, list_catalogs, list_schemas, list_tables, get_table_schema) must return JSON-formatted responses and handle parameter validation.
Applied to files:
README.md
🪛 markdownlint-cli2 (0.17.2)
README.md
876-876: Bare URL used
(MD034, no-bare-urls)
🔇 Additional comments (6)
internal/oauth/metadata.go (3)
85-89: Excellent URL construction refactor.The consistent use of
h.config.MCPURLfor all OAuth endpoints ensures proper URL generation for remote deployments and improves maintainability by centralizing URL construction logic.Also applies to: 94-94
117-118: Consistent URL construction implementation.The refactoring to use
h.config.MCPURLmaintains consistency across all protected resource metadata endpoints and supports proper remote deployment configuration.Also applies to: 121-124
154-162: Clean formatting improvements.The alignment and formatting changes improve code readability while maintaining all existing functionality.
README.md (3)
710-710: Essential documentation for remote deployments.This addition clearly explains when
MCP_URLis required and why it's important for OAuth metadata and client discovery. This directly supports the code changes that centralize URL construction.
718-718: Helpful production deployment example.Adding
MCP_URLto the production deployment example provides clear guidance for users configuring remote MCP servers.
876-876: Complete configuration documentation.The addition of
MCP_URLto the configuration table provides users with clear information about the variable's purpose and default value, supporting the centralized URL configuration approach.
| "supported_flows": []string{"claude_code", "mcp_remote"}, | ||
| "mcp_version": "1.0.0", | ||
| "server_version": h.config.Version, | ||
| "provider": h.config.Provider, | ||
| "authorization_endpoint": fmt.Sprintf("%s://%s:%s/oauth/authorize", h.config.Scheme, h.config.MCPHost, h.config.MCPPort), |
There was a problem hiding this comment.
Inconsistent URL construction in metadata endpoint.
While other metadata endpoints have been updated to use h.config.MCPURL, this authorization endpoint still uses manual URL construction. This could lead to inconsistent URLs being returned by different metadata endpoints.
Apply this diff to maintain consistency:
- "authorization_endpoint": fmt.Sprintf("%s://%s:%s/oauth/authorize", h.config.Scheme, h.config.MCPHost, h.config.MCPPort),
+ "authorization_endpoint": fmt.Sprintf("%s/oauth/authorize", h.config.MCPURL),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "authorization_endpoint": fmt.Sprintf("%s://%s:%s/oauth/authorize", h.config.Scheme, h.config.MCPHost, h.config.MCPPort), | |
| "authorization_endpoint": fmt.Sprintf("%s/oauth/authorize", h.config.MCPURL), |
🤖 Prompt for AI Agents
In internal/oauth/metadata.go at line 42, the authorization_endpoint URL is
manually constructed using scheme, host, and port, unlike other metadata
endpoints that use h.config.MCPURL. To fix this inconsistency, replace the
manual URL construction with h.config.MCPURL concatenated with
"/oauth/authorize" to ensure all metadata endpoints use the same base URL
format.
* PE-7046: Deploy Trino MCP Server * PE-7046: Fix workflow * PE-7046: Fix workflow * PE-7046: Fix workflow * remove default annotations for ALB ingress controller * PE-7208: Support hostname in MCP URL (#3) * PE-7208: Support hostname in MCP URL Signed-off-by: Yang Wang <wyangsun@163.com> * Update oauth.md Signed-off-by: Yang Wang <wyangsun@163.com> * Update oauth.md Signed-off-by: Yang Wang <wyangsun@163.com> --------- Signed-off-by: Yang Wang <wyangsun@163.com> --------- Signed-off-by: Yang Wang <wyangsun@163.com>
Summary by CodeRabbit
Summary by CodeRabbit
MCP_URLto README for configuring public MCP server URL.