Skip to content

Fix broken test, upgrade to golangci-lint@v2.1.1 and fix numerous lint errors, upgrade tool versions, remove hack/tools.go, etc.#64

Merged
unmarshall merged 13 commits into
ai-dynamo:mainfrom
renormalize:dev-productivity
Apr 16, 2025
Merged

Fix broken test, upgrade to golangci-lint@v2.1.1 and fix numerous lint errors, upgrade tool versions, remove hack/tools.go, etc.#64
unmarshall merged 13 commits into
ai-dynamo:mainfrom
renormalize:dev-productivity

Conversation

@renormalize

@renormalize renormalize commented Apr 15, 2025

Copy link
Copy Markdown
Contributor
  • Fix broken test TestDefaultPodGangSet in operator/internal/webhook/admission/pgs/defaulting/podgangset_test.go.
  • Remove scheduler-api/hack/tools.go, and use the tool section in scheduler-api/go.mod, following the Go 1.24 convention, making it uniform across all directories in the repository.
  • Migrate to golangci-lint@v2.1.1 (major version upgrade).
  • Fix numerous lint errors raised by golangci-lint.
    • Comments on exported tokens.
    • Renaming unused variables to _. These unused variables can be renamed to appropriate names when necessary, as most are trivial. Added suggestions in places where deemed necessary.
  • make targets are made holistic by replicating wherever necessary.
  • make generate wherever applicable.
  • Upgrade to goimports-reviser@3.9.1 since older versions are not supported by Go 1.24.
  • Upgrade to kind@v0.27.0.
  • Revise imports in all files.

NOTE: make generate in scheduler-api fails, which has an easy enough fix, but affects the API so I refrained from making the change there. Input appreciated @unmarshall @sanjaychatterjee.
The type NamespacedName does not include JSON tags; therefore the field PodReferences in the struct:

// PodGroup defines a set of pods in a PodGang that share the same PodTemplateSpec.
type PodGroup struct {
	// PodReferences is a list of references to the Pods that are part of this group.
	PodReferences []types.NamespacedName `json:"podReferences"`
	// MinReplicas is the number of replicas that needs to be gang scheduled.
	// If the MinReplicas is greater than len(PodReferences) then scheduler makes the best effort to schedule as many pods beyond
	// MinReplicas. However, guaranteed gang scheduling is only provided for MinReplicas.
	MinReplicas int32 `json:"minReplicas"`
}

will cause issues while generating code. controller-gen expects each struct's fields to have JSON tags, and this imported type from k8s.io/apimachinery/pkg/types does not.

  • A potential solution is to define:

    type OwnNamespacedName struct {
        Namespace string `json:"namespace"`
        Name      string `json:"name"`
    }

    in the same package, and then change PodGroup to:

    // PodGroup defines a set of pods in a PodGang that share the same PodTemplateSpec.
    type PodGroup struct {
        PodReferences []OwnNamespacedName `json:"podReferences"`
        MinReplicas int32 `json:"minReplicas"`
    }

    I've done the following locally and have verified that make generate succeeds.

This project has done the same.


Given the size of the PR, I've structured it such that it will be trivial to review commit wise.
I would highly recommend doing so since quite a few commits can be skipped as they are generated.

PLEASE SQUASH AND MERGE, DO NOT CREATE A MERGE COMMIT OR REBASE AND MERGE as there are multiple commits, to avoid polluting the git history in main.

Signed-off-by: Saketh Kalaga <51327242+renormalize@users.noreply.github.com>
* Upgrade to `goimports-reviser@3.9.1` to support Go 1.24.

* Upgrade `kind` to `v0.27.0`.

* Upgrade `yq` to `v4.45.1`.

Signed-off-by: Saketh Kalaga <51327242+renormalize@users.noreply.github.com>
Signed-off-by: Saketh Kalaga <51327242+renormalize@users.noreply.github.com>
Signed-off-by: Saketh Kalaga <51327242+renormalize@users.noreply.github.com>
Signed-off-by: Saketh Kalaga <51327242+renormalize@users.noreply.github.com>
Signed-off-by: Saketh Kalaga <51327242+renormalize@users.noreply.github.com>
…cheduler-api/`.

* Introduce the `lint` make target, and resolve a lint error.

* Introduce the `format` make target, with the corresponding
`hack/format.sh`.

* Introduce the `add-license-headers` make target, with the
corresponding `hack/add-license-headers.sh`.

Signed-off-by: Saketh Kalaga <51327242+renormalize@users.noreply.github.com>
Signed-off-by: Saketh Kalaga <51327242+renormalize@users.noreply.github.com>
…r-plugins/`.

* Introduce the `format` make target along with the corresponding
`hack/format.sh`.

* Introduce the `add-license-headers` make target, with the
corresponding `hack/add-license-headers.sh`.

* Correct `boilderplate.go.txt` to be the same as the files in the other
directories.

Signed-off-by: Saketh Kalaga <51327242+renormalize@users.noreply.github.com>
Signed-off-by: Saketh Kalaga <51327242+renormalize@users.noreply.github.com>
Signed-off-by: Saketh Kalaga <51327242+renormalize@users.noreply.github.com>
Signed-off-by: Saketh Kalaga <51327242+renormalize@users.noreply.github.com>
…section in `go.mod`.

Signed-off-by: Saketh Kalaga <51327242+renormalize@users.noreply.github.com>
@renormalize renormalize added enhancement New feature or request dependencies Pull requests that update a dependency file go Pull requests that update go code labels Apr 15, 2025
@unmarshall

Copy link
Copy Markdown
Collaborator

I have raised kubernetes/kubernetes#131313 if there is no objection from k8s apimachinery sig then i will also raise a PR soon.

@unmarshall unmarshall merged commit acbbfd5 into ai-dynamo:main Apr 16, 2025
@renormalize renormalize deleted the dev-productivity branch April 16, 2025 03:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

dependencies Pull requests that update a dependency file enhancement New feature or request go Pull requests that update go code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants