Skip to content

Conversation

@alexzielenski
Copy link
Member

@alexzielenski alexzielenski commented Aug 17, 2023

Continuation of #240 which went stale and closed.

Files whose names begin with "_" are ignored by go test so none of the defaulter-gen tests were running in CI.
[Some] tests failed because some packages missed a fake_deepcopy_conversion.go file. This is necessary because types registered in the scheme need to implement the DeepCopyObject() method.

Took into account some of the feedback provided on that PR, keeping _output-tests but adding its own go.mod so there can be a dependency on k/k packages directly without forming a cycle.

Changes from initial PR:

  • Move output_tests back to _output_tests
  • Added go.mod to _output_tests directory
  • Rename package path from _output_tests to output_tests - otherwise retaining underscore in go package name caused ambiguous import error with gengo's file tree:
    The module name itself still needed the underscore removed otherwise would get this error:
ambiguous import: found package k8s.io/gengo/examples/defaulter-gen/_output_tests/empty in multiple modules:
        k8s.io/gengo v0.0.0 ($GOMOD/k8s.io/[email protected]/examples/defaulter-gen/_output_tests/empty)
        k8s.io/gengo/examples/defaulter-gen/_output_tests (/Users/alex/go/src/k8s.io/gengo/examples/defaulter-gen/_output_tests/empty)
ambiguous import: found package k8s.io/gengo/examples/defaulter-gen/_output_tests/marker in multiple modules:
        k8s.io/gengo v0.0.0 ($GOMOD/k8s.io/[email protected]/examples/defaulter-gen/_output_tests/marker)
        k8s.io/gengo/examples/defaulter-gen/_output_tests (/Users/alex/go/src/k8s.io/gengo/examples/defaulter-gen/_output_tests/marker)
ambiguous import: found package k8s.io/gengo/examples/defaulter-gen/_output_tests/marker/external in multiple modules:
        k8s.io/gengo v0.0.0 ($GOMOD/k8s.io/[email protected]/examples/defaulter-gen/_output_tests/marker/external)
        k8s.io/gengo/examples/defaulter-gen/_output_tests (/Users/alex/go/src/k8s.io/gengo/examples/defaulter-gen/_output_tests/marker/external)
ambiguous import: found package k8s.io/gengo/examples/defaulter-gen/_output_tests/marker/external/external in multiple modules:
        k8s.io/gengo v0.0.0 ($GOMOD/k8s.io/[email protected]/examples/defaulter-gen/_output_tests/marker/external/external)
        k8s.io/gengo/examples/defaulter-gen/_output_tests (/Users/alex/go/src/k8s.io/gengo/examples/defaulter-gen/_output_tests/marker/external/external)
ambiguous import: found package k8s.io/gengo/examples/defaulter-gen/_output_tests/pointer in multiple modules:
        k8s.io/gengo v0.0.0 ($GOMOD/k8s.io/[email protected]/examples/defaulter-gen/_output_tests/pointer)
        k8s.io/gengo/examples/defaulter-gen/_output_tests (/Users/alex/go/src/k8s.io/gengo/examples/defaulter-gen/_output_tests/pointer)
ambiguous import: found package k8s.io/gengo/examples/defaulter-gen/_output_tests/slices in multiple modules:
        k8s.io/gengo v0.0.0 ($GOMOD/k8s.io/[email protected]/examples/defaulter-gen/_output_tests/slices)
        k8s.io/gengo/examples/defaulter-gen/_output_tests (/Users/alex/go/src/k8s.io/gengo/examples/defaulter-gen/_output_tests/slices)
ambiguous import: found package k8s.io/gengo/examples/defaulter-gen/_output_tests/wholepkg in multiple modules:
        k8s.io/gengo v0.0.0 ($GOMOD/k8s.io/[email protected]/examples/defaulter-gen/_output_tests/wholepkg)
        k8s.io/gengo/examples/defaulter-gen/_output_tests (/Users/alex/go/src/k8s.io/gengo/examples/defaulter-gen/_output_tests/wholepkg)
make: *** [test] Error 1

I think since this PR adds a new go module CI would need to be updated to run it. This PR makes go test ./... from within _output_tests correctly run without hacks.

nikhita and others added 4 commits January 18, 2023 18:02
Files whose names begin with "_" are ignored by go test so none of the
defaulter-gen tests were running in CI.

This commit renames `_output_tests` to `output_tests` so that tests are
run.

After doing so, tests failed because some packages missed a
`fake_deepcopy_conversion.go` file. This is necessary because types
registered in the scheme need to implement the `DeepCopyObject()` method.

Additionally, some of the tests import `k8s.io/api` and `k8s.io/apimachinery`
so these are also added to `go.mod`.
@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Aug 17, 2023
@k8s-ci-robot k8s-ci-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Aug 17, 2023
@alexzielenski alexzielenski changed the title be able to run tests via go test be able to run defaulter-get tests via go test Aug 17, 2023
@alexzielenski alexzielenski changed the title be able to run defaulter-get tests via go test be able to run defaulter-gen tests via go test Aug 17, 2023
@alexzielenski
Copy link
Member Author

/cc @sttts

Copy link
Member

@apelisse apelisse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, that's definitely confusing. Can we get a comment somewhere to indicate what's going on without having to go through the github history?

@alexzielenski
Copy link
Member Author

alexzielenski commented Aug 17, 2023

updated PR description and added comment

@alexzielenski
Copy link
Member Author

/label tide/merge-method-squash

@k8s-ci-robot k8s-ci-robot added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Aug 22, 2023
@apelisse
Copy link
Member

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 22, 2023
@sttts
Copy link
Contributor

sttts commented Aug 23, 2023

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alexzielenski, apelisse, sttts

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 23, 2023
@k8s-ci-robot k8s-ci-robot merged commit c57d208 into kubernetes:master Aug 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants