Support non-Infra GKE cluster type and allow Unknown as cluster type#254
Conversation
…as a generic type explicitly
📝 WalkthroughWalkthroughCluster type handling now supports general GKE contexts, represents unknown values as ChangesCluster type flow
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant configureConfig
participant DetectClusterType
participant ClusterType
configureConfig->>DetectClusterType: request auto-detected cluster type
DetectClusterType->>ClusterType: classify gke_ context as GKE
ClusterType-->>configureConfig: return ClusterTypeGKE
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the 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. Comment |
|
@coderabbitai, try again? |
|
✅ Action performedReview finished.
|
|
@coderabbitai, still rate limited? |
|
🐇💭 ✅ Action performedReview finished.
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/env/env.go (1)
147-152: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider using
contextLowerfor consistency.The rest of the heuristics in this function rely on
contextLowerfor case-insensitive matching. Using it here makes the GKE checks more resilient and consistent with the other cluster type checks.♻️ Proposed refactor
- if strings.HasPrefix(config.CurrentContext, "gke_acs-team-temp-dev") { + if strings.HasPrefix(contextLower, "gke_acs-team-temp-dev") { return types.ClusterTypeInfraGKE } - if strings.HasPrefix(config.CurrentContext, "gke_") { + if strings.HasPrefix(contextLower, "gke_") { return types.ClusterTypeGKE }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/env/env.go` around lines 147 - 152, Update the GKE prefix checks in the cluster-type detection function to use the existing contextLower value instead of config.CurrentContext, preserving the existing infra-GKE precedence and return values while ensuring case-insensitive matching.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@internal/env/env.go`:
- Around line 147-152: Update the GKE prefix checks in the cluster-type
detection function to use the existing contextLower value instead of
config.CurrentContext, preserving the existing infra-GKE precedence and return
values while ensuring case-insensitive matching.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Enterprise
Run ID: d5422cff-cc29-48ec-8200-f61efdff7af1
📒 Files selected for processing (7)
cmd/deploy.gointernal/clusterdefaults/clusterdefaults.gointernal/clusterdefaults/clusterdefaults_test.gointernal/env/env.gointernal/env/env_test.gointernal/types/cluster_type.gointernal/types/cluster_type_test.go
Support non-Infra GKE cluster type and also support specifying 'Unknown' as a generic type explicitly
Summary by CodeRabbit