fix: track ui/dist, correct CI action versions, add API key support and Windows gcloud fix#289
Conversation
…ules The dist/ gitignore pattern was also excluding ui/dist/, causing the //go:embed directives in ui/ui.go to fail when the module is downloaded from the Go proxy (missing dist/apps/dropdown/index.html). Change dist/ to /dist/ so only the goreleaser root dist/ is excluded, and track the built UI files so they are included in published module versions. Also add gke-mcp.exe and gke-mcp.exe~ to .gitignore. Generated with oh-my-agent Co-Authored-By: First Fluke <our.first.fluke@gmail.com>
- Downgrade actions/checkout v6→v4, actions/setup-go v6→v5, actions/setup-node v6→v4, goreleaser-action v7→v6 (v6/v7 do not exist) - Add validate-skills job to presubmit.yaml to match dev/tasks/presubmit.sh Generated with oh-my-agent Co-Authored-By: First Fluke <our.first.fluke@gmail.com>
…ows gcloud fix - Read GOOGLE_API_KEY / GEMINI_API_KEY and fall back to Gemini API backend in manifestgen when no GCP project is set - Resolve project/location from GKE_MCP_PROJECT, GOOGLE_CLOUD_PROJECT, GCP_PROJECT, GKE_MCP_LOCATION, GOOGLE_CLOUD_LOCATION, GCP_LOCATION env vars before invoking gcloud, and treat "(unset)" as empty - Default location to us-central1 when nothing is configured - Use cmd /c gcloud on Windows so gcloud.cmd is found on PATH - Switch path/filepath → path for embed.FS joins (always forward slashes) Generated with oh-my-agent Co-Authored-By: First Fluke <our.first.fluke@gmail.com>
Generated with oh-my-agent Co-Authored-By: First Fluke <our.first.fluke@gmail.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: sonnyboybacus-cyber The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces support for Gemini API keys, enhances environment variable detection for project and location configurations, and improves Windows compatibility for gcloud commands. It also updates the .gitignore and dependency lock files. Feedback suggests removing the hardcoded default location 'us-central1' to require explicit configuration and recommends using the idiomatic 'runtime.GOOS' for more reliable operating system detection.
| return zone | ||
| } | ||
| return "" | ||
| return "us-central1" |
There was a problem hiding this comment.
The function getDefaultLocation returns a hardcoded default value "us-central1" when no location is found in the environment or gcloud configuration. This violates the general rule that tools should not fall back to default values for parameters like project and location, and instead expect the calling agent to provide them explicitly.
| return "us-central1" | |
| return "" |
References
- Tools should not fall back to default values for parameters like project and location. The calling agent is expected to provide these values explicitly.
| out, err := exec.Command("gcloud", "config", "get", key).Output() | ||
| // Use 'cmd /c gcloud' on Windows to ensure it finds gcloud.cmd or gcloud.ps1 | ||
| var cmd *exec.Cmd | ||
| if os.Getenv("OS") == "Windows_NT" { |
There was a problem hiding this comment.
Detecting the operating system via the OS environment variable is unreliable as it may not be set or could be modified. The idiomatic and robust way to check the operating system in Go is using runtime.GOOS.
bradhoekstra
left a comment
bradhoekstra
left a comment
There was a problem hiding this comment.
Thank you for the PR @sonnyboybacus-cyber ! I've left some comments, overall it would be simpler to review if the different unrelated changes were split into their own PRs.
| permissions: | ||
| contents: "read" | ||
| steps: | ||
| - uses: actions/checkout@v6 |
There was a problem hiding this comment.
These versions do exist, otherwise our CI/CD would be failing:
https://github.com/actions/checkout
https://github.com/actions/setup-go
| } | ||
| }, | ||
| "name": "gke-mcp", | ||
| "version": "0.11.1" |
There was a problem hiding this comment.
Version changes should be their own PR.
There was a problem hiding this comment.
lgtm on the manifest gen agent change.
3 participants
Summary
ui/dist/build artifacts —dist/in.gitignorewas also excludingui/dist/, so//go:embedinui/ui.gofailed when the module was downloaded from the Go proxy. Changeddist/→/dist/and committed the built UI files.checkout@v6,setup-go@v6,setup-node@v6,goreleaser-action@v7do not exist; downgraded to valid versions. Added missingvalidate-skillsCI job.GOOGLE_API_KEY/GEMINI_API_KEY, fall back to Gemini API when no GCP project is set; resolve config from env vars before gcloud; default location tous-central1; usecmd /c gcloudon Windows; fixembed.FSpath joins.0.11.2Test plan
go run github.com/GoogleCloudPlatform/gke-mcp@latestbuilds after mergegke-mcpworks withGOOGLE_API_KEYand no gcloud projectgke-mcpworks on Windows