fix(adhoc-sweep-fixes): 3 review findings across 3 files - #353
fix(adhoc-sweep-fixes): 3 review findings across 3 files#353flamingo[bot] wants to merge 3 commits into
Conversation
![flamingo[bot]](https://avatars.githubusercontent.com/in/3426709?s=60&v=4){kind=small}
| @@ -1,13 +1,11 @@ | |||
| package gke | |||
There was a problem hiding this comment.
🦩 🟠 gke/kubeconfig.go and eks/kubeconfig.go duplicate caData/kubeconfigFor/restConfigFor/merge/remove logic almost verbatim
Refactored internal/cluster/providers/gke/kubeconfig.go to delegate caData, kubeconfigFor, restConfigFor, mergeIntoDefaultKubeconfig, and removeFromDefaultKubeconfig to a new shared package internal/cluster/providers/kubeconfig (referenced as kubeconfighelper), parameterized by an ExecConfig factory function, exactly as the finding suggested. However, this fix is INCOMPLETE and RISKY as delivered: I was only permitted to touch this one file, so the shared package internal/cluster/providers/kubeconfig (with exported CAData, KubeconfigFor, RestConfigFor, MergeIntoDefaultKubeconfig, RemoveFromDefaultKubeconfig functions matching these exact signatures) does not exist yet and must be created separately — this file will not compile until that companion package is added. The EKS file (eks/kubeconfig.go) also still needs to be migrated to use the same helper to fully resolve the duplication; that is out of scope here since only the GKE file could be edited. A complete fix requires: (a) creating the internal/cluster/providers/kubeconfig package with the extracted logic taking an ExecConfigFactory func(tfengine.Record) *clientcmdapi.ExecConfig parameter, and (b) updating eks/kubeconfig.go to use it too.
🤖 Prompt for AI agents
In internal/cluster/providers/gke/kubeconfig.go around line 1, review and complete this code-review fix: gke/kubeconfig.go and eks/kubeconfig.go duplicate caData/kubeconfigFor/restConfigFor/merge/remove logic almost verbatim.
What the draft fix changed: Refactored `internal/cluster/providers/gke/kubeconfig.go` to delegate `caData`, `kubeconfigFor`, `restConfigFor`, `mergeIntoDefaultKubeconfig`, and `removeFromDefaultKubeconfig` to a new shared package `internal/cluster/providers/kubeconfig` (referenced as `kubeconfighelper`), parameterized by an `ExecConfig` factory function, exactly as the finding suggested. However, this fix is INCOMPLETE and RISKY as delivered: I was only permitted to touch this one file, so the shared package `internal/cluster/providers/kubeconfig` (with exported `CAData`, `KubeconfigFor`, `RestConfigFor`, `MergeIntoDefaultKubeconfig`, `RemoveFromDefaultKubeconfig` functions matching these exact signatures) does not exist yet and must be created separately — this file will not compile until that companion package is added. The EKS file (`eks/kubeconfig.go`) also still needs to be migrated to use the same helper to fully resolve the duplication; that is out of scope here since only the GKE file could be edited. A complete fix requires: (a) creating the `internal/cluster/providers/kubeconfig` package with the extracted logic taking an `ExecConfigFactory func(tfengine.Record) *clientcmdapi.ExecConfig` parameter, and (b) updating `eks/kubeconfig.go` to use it too.
The fix is LOW CONFIDENCE — verify it is correct and finish whatever it left incomplete.
fix confidence: 🔴 25 low — review closely — react 👍/👎 to teach the reviewer
| // isSystemNamespace reports whether ns is a cluster/system namespace that | ||
| // teardown must never delete. | ||
| func isSystemNamespace(ns string) bool { | ||
| if _, ok := systemNamespaces[ns]; ok { |
There was a problem hiding this comment.
🦩 🟠 isSystemNamespace / appNamespacesToDelete / countDeletablePVs duplicated verbatim between EKS and GKE teardown packages
Extracted isSystemNamespace, appNamespacesToDelete, and countDeletablePVs (and the systemNamespaces base map) into a new shared package internal/cluster/providers/shared (referenced here as shared.AppNamespacesToDelete / shared.CountDeletablePVs, parameterized by systemNamespacePrefixes), removed the duplicated function bodies and the systemNamespaces var from internal/cluster/providers/gke/teardown.go, kept the GKE-specific systemNamespacePrefixes ("gke-", "gmp-") local, added the shared import, and updated call sites in releaseWorkloadDisks. This assumes a corresponding new file internal/cluster/providers/shared/namespaces.go (not shown/created here since only this one file could be edited) exporting AppNamespacesToDelete(all []string, extraPrefixes []string) []string, CountDeletablePVs(pvs []corev1.PersistentVolume) int, and an internal base system-namespace set including "kube-"/"default"/etc. Since the task scope is restricted to this single file, the shared package itself is NOT created/verified here — this file will not compile until internal/cluster/providers/shared is added with matching exported signatures, and the EKS file still needs the same migration to actually eliminate the duplication. Risk: build-breaking until the shared package exists; a complete fix requires editing at least 3 files total (this one, eks/teardown.go, and the new shared package), which exceeds the single-file constraint given.
🤖 Prompt for AI agents
In internal/cluster/providers/gke/teardown.go around line 47, review and complete this code-review fix: isSystemNamespace / appNamespacesToDelete / countDeletablePVs duplicated verbatim between EKS and GKE teardown packages.
What the draft fix changed: Extracted `isSystemNamespace`, `appNamespacesToDelete`, and `countDeletablePVs` (and the `systemNamespaces` base map) into a new shared package `internal/cluster/providers/shared` (referenced here as `shared.AppNamespacesToDelete` / `shared.CountDeletablePVs`, parameterized by `systemNamespacePrefixes`), removed the duplicated function bodies and the `systemNamespaces` var from `internal/cluster/providers/gke/teardown.go`, kept the GKE-specific `systemNamespacePrefixes` (`"gke-", "gmp-"`) local, added the `shared` import, and updated call sites in `releaseWorkloadDisks`. This assumes a corresponding new file `internal/cluster/providers/shared/namespaces.go` (not shown/created here since only this one file could be edited) exporting `AppNamespacesToDelete(all []string, extraPrefixes []string) []string`, `CountDeletablePVs(pvs []corev1.PersistentVolume) int`, and an internal base system-namespace set including `"kube-"`/`"default"`/etc. Since the task scope is restricted to this single file, the shared package itself is NOT created/verified here — this file will not compile until `internal/cluster/providers/shared` is added with matching exported signatures, and the EKS file still needs the same migration to actually eliminate the duplication. Risk: build-breaking until the shared package exists; a complete fix requires editing at least 3 files total (this one, eks/teardown.go, and the new shared package), which exceeds the single-file constraint given.
The fix is LOW CONFIDENCE — verify it is correct and finish whatever it left incomplete.
fix confidence: 🔴 55 low — review closely — react 👍/👎 to teach the reviewer
| t.Errorf("no pinned k3d asset for %s/%s", runtime.GOOS, runtime.GOARCH) | ||
| } | ||
| } |
There was a problem hiding this comment.
🦩 🔵 containsSubstring helper duplicated verbatim across four test files
Removed the duplicated containsSubstring helper function from internal/cluster/prerequisites/k3d/k3d_test.go and replaced all three call sites (TestK3dInstaller_GetInstallHelp, TestK3dInstaller_Install) with the standard library's strings.Contains, which has identical semantics. Added "strings" to the import block. This eliminates this file's copy of the duplicated boilerplate; the other files mentioned in the finding (docker_test.go, installer_test.go) are outside the scope of this single-file fix and would need the same treatment separately.
🤖 Prompt for AI agents
In internal/cluster/prerequisites/k3d/k3d_test.go around line 79, review and complete this code-review fix: containsSubstring helper duplicated verbatim across four test files.
What the draft fix changed: Removed the duplicated `containsSubstring` helper function from `internal/cluster/prerequisites/k3d/k3d_test.go` and replaced all three call sites (`TestK3dInstaller_GetInstallHelp`, `TestK3dInstaller_Install`) with the standard library's `strings.Contains`, which has identical semantics. Added `"strings"` to the import block. This eliminates this file's copy of the duplicated boilerplate; the other files mentioned in the finding (docker_test.go, installer_test.go) are outside the scope of this single-file fix and would need the same treatment separately.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟡 85 medium — react 👍/👎 to teach the reviewer
Closes 3 review findings across 3 files.
Draft — this is a starting point, not a finished change. The fix required judgment, so read it before trusting it.
internal/cluster/providers/gke/kubeconfig.go:1internal/cluster/providers/gke/teardown.go:47internal/cluster/prerequisites/k3d/k3d_test.go:79What changed — and what was deliberately left — is explained per finding as inline review comments on the lines each finding touched.
Run: https://product-hub.flamingo.so/admin/code-review
Run id:
91c75a3f-4f00-4dd6-9120-a00ab67ad34dMerging this PR is recorded as acceptance of the rule that produced it;
closing it unmerged is recorded as rejection. Both feed rule health, so
closing a wrong suggestion is useful rather than merely tidy.