kubectl-datadog: redistribute autoscaling cluster packages by concern#3012
Conversation
kubectl-datadog: redistribute autoscaling cluster packages by concern
|
@codex review |
|
Codex Review: Didn't find any major issues. Keep it up! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
🛑 Gate Violations
ℹ️ Info🎯 Code Coverage (details) Useful? React with 👍 / 👎 This comment will be updated automatically if new data arrives.🔗 Commit SHA: ffb2176 | Docs | Datadog PR Page | Give us feedback! |
|
Note on the failing This advisory (non-blocking) gate reports ~9% patch coverage. The "uncovered" lines are the import-path updates in No new logic was introduced by this PR; every moved function retained its existing tests (see |
clamoriniere
left a comment
There was a problem hiding this comment.
Approve again after PR move to ready state
Continue the redistribution of `guess/` started by PRs #2980 and #3000: move every helper that operates on an EKS cluster object or the EKS API into a dedicated `common/eks/` package. After this commit, `common/eks/` contains: - authmode.go (was guess/clusterauthmode.go) - podidentityagent.go (was guess/ekspodidentityagent.go) - oidcprovider.go (was guess/oidcprovider.go) - privatesubnets.go (was guess/privatesubnets.go) Import alias `commoneks` is used to avoid colliding with the existing `github.com/aws/aws-sdk-go-v2/service/eks` (aliased `eks`). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
`common/aws/aws-auth.go` was misnamed: it manipulates a Kubernetes ConfigMap (`kube-system/aws-auth`), not an AWS-SDK resource. Together with `guess/aws-auth.go` (read-only presence check), it belongs in a dedicated `common/awsauth/` package: both files operate on the same ConfigMap. Function names lose the now-redundant `AwsAuth` prefix: - `IsAwsAuthConfigMapPresent` -> `IsConfigMapPresent` - `EnsureAwsAuthRole` -> `EnsureRole` - `RemoveAwsAuthRole` -> `RemoveRole` `common/aws/` now contains only cloudformation.go (pure AWS-SDK). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The only caller of `guess.GetClusterNameFromKubeconfig` is the wrapper `clients.GetClusterNameFromKubeconfig` in `common/clients/clients.go`. Move the helper next to its caller as an unexported function `clusterNameFromKubeconfig`. The public API of `common/clients/` is unchanged. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Consolidate everything Karpenter-shaped into `common/karpenter/`,
alongside the existing detection helper:
- Intermediate model: NodePoolsSet, EC2NodeClass, NodePool,
MetadataOptions, BlockDeviceMapping (from guess/nodepoolsset.go)
- Producers from EKS node groups: GetNodeGroupsProperties (from
guess/nodegroupproperties.go -> fromnodegroups.go)
- Producers from running k8s Nodes: GetNodesProperties (from
guess/nodesproperties.go -> fromnodes.go)
- CR builders: CreateOrUpdateEC2NodeClass, CreateOrUpdateNodePool
(from cluster/k8s/{ec2nodeclass,nodepool}.go)
`cluster/k8s/` is gone (it was misnamed: those files build Karpenter
CRDs, they aren't generic k8s primitives). `common/k8s/` keeps its
narrow scope: generic object CRUD + deployment-finder.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
`kube-system` and `aws-auth` each appeared 5 times across the three functions. Promote them to package-level constants so the package's single-ConfigMap scope is explicit at a glance. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
b2fc6f5 to
ffb2176
Compare
What does this PR do?
Continues the refactoring started by PRs #2980 and #3000 of the
cmd/kubectl-datadog/autoscaling/cluster/tree: empties theguess/package and removes the misnamedcluster/k8s/package by redistributing their contents into focusedcommon/<concern>/packages.Final layout under
common/:aws/cloudformation.goawsauth/kube-system/aws-authConfigMap (was split betweenguess/aws-auth.goand the misplacedcommon/aws/aws-auth.go)clients/guess.GetClusterNameFromKubeconfig, folded as unexported)clusterautoscaler/clusterinfo/display/eks/authmode.go,oidcprovider.go,podidentityagent.go,privatesubnets.go(wasguess/)eksautomode/helm/k8s/object.go(CRUD),deployment.go(finder)karpenter/nodepoolsset.go) + producers (fromnodes.go,fromnodegroups.go) + CR builders (ec2nodeclass.go,nodepool.go)After this PR,
guess/no longer exists andcluster/k8s/no longer exists.Motivation
common/aws/aws-auth.gowas misnamed: it manipulates a Kubernetes ConfigMap, not an AWS-SDK resource. Together with the read-only detection inguess/aws-auth.go, it belongs in a dedicatedcommon/awsauth/package.cluster/k8s/was misnamed: its two files (ec2nodeclass.go,nodepool.go) build Karpenter CRDs. Karpenter-specific code belongs alongside the other Karpenter code incommon/karpenter/.guess/files mixed AWS-SDK and Kubernetes API concerns under a name that no longer described any abstraction. Each file moves to a concern-focused package (common/eks/for EKS-API helpers;common/karpenter/for Karpenter-shape inference;common/clients/for kubeconfig parsing).common/aws/andcommon/k8s/hold generic primitives only; every concrete concern (EKS, Karpenter, aws-auth, …) gets its owncommon/<concern>/package. This continues the pattern PRs [CASCL-1304]kubectl-datadog: enrichdd-cluster-infoConfigMap #2980 and [CASCL-1304] Fix Fargate profile name indd-cluster-infoConfigMap #3000 introduced.Branched off
lenaic/merge_2980_3000(which bundles the in-flight PRs #2980 and #3000) to minimize merge conflicts.Additional Notes
common/awsauth/strip the now-redundantAwsAuthprefix:IsAwsAuthConfigMapPresent→IsConfigMapPresent,EnsureAwsAuthRole→EnsureRole,RemoveAwsAuthRole→RemoveRole.RoleMappingis unchanged.common/eks/is imported ascommonekseverywhere (alias) to avoid colliding with the AWS SDK'sgithub.com/aws/aws-sdk-go-v2/service/eksalready aliasedeks. Same idiom as the existingcommonaws/commonk8saliases.guess.GetClusterNameFromKubeconfighad a single caller (the wrapper incommon/clients/clients.go) — moved next to it as the unexportedclusterNameFromKubeconfig. The publicclients.GetClusterNameFromKubeconfigAPI is unchanged.kubectl-datadog: enrichdd-cluster-infoConfigMap #2980 and [CASCL-1304] Fix Fargate profile name indd-cluster-infoConfigMap #3000 are merged. Base branch will be re-pointed tomainonce those land.Minimum Agent Versions
Not applicable — no behavior change.
Describe your test plan
go build ./...— passesgo vet ./...— passesgo test ./cmd/kubectl-datadog/autoscaling/cluster/...— all packages passmake kubectl-datadog(includesgolangci-lint) — 0 issuesNo behavior change is intended; existing unit tests cover the moved functions. Manual end-to-end smoke (
autoscaling cluster install/update/uninstallagainst a test EKS cluster) recommended before merge to validate that no call site was missed.Checklist
refactoringqa/skip-qalabel