Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add verify-examples.sh and switch from makefiles to go test and go generate #249

Merged
merged 8 commits into from
Aug 24, 2023

Conversation

alexzielenski
Copy link
Contributor

@alexzielenski alexzielenski commented Aug 23, 2023

This PR:

  • Adds generate.go to deepcopy-gen and defaulter-gen, and fixes the set-gen generate to not rely on GOPATH. This enables the more standard go generate to be used over ad-hoc makefiles
  • Adds hack/verify-examples.sh which will be called from CI to:
    1.  Remove generated code
    2. Re-generate code
    3. Run output tests
    4. Check for diff with git
  • Changes most example tests to specify the boilerplate header path. Otherwise would rely on having k8s.io/gengo in your GOPATH to run
  • Removes all makefiles in favor of go generate and go test workflows

After this PR we will be able to regenerate and rerun all examples tests in CI by invoking hack/verify-examples.sh

/cc @apelisse

@k8s-ci-robot k8s-ci-robot requested a review from apelisse August 23, 2023 21:36
@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Aug 23, 2023
@apelisse
Copy link
Member

Awesome, can't wait to see the CI!
/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 23, 2023
@alexzielenski
Copy link
Contributor Author

/hold

still making this better

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 23, 2023
provides additional header path argument to make gen so that it is portable without having k8s.io/gengo in your GOPATH.

also ensures defaulter-gen is run from within _output_tests to fix bug where packages were no longer being processed unless a go.work file is present
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 23, 2023
@alexzielenski alexzielenski changed the title defaulter-gen: Make make gen more portable Add verify-examples.sh and make example gen not rely on GOPATH Aug 23, 2023
@apelisse
Copy link
Member

I don't know how I feel about having both the makefile and the shell script?

@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Aug 23, 2023
@alexzielenski
Copy link
Contributor Author

I don't know how I feel about having both the makefile and the shell script?

agreed. removed the makefiles since they are supplanted by the verify script. However import-boss still retains a makefile since its test is quite different. I could refactor it to run as part of go test but it would have to just be a test that invokes go run of it self

@alexzielenski
Copy link
Contributor Author

/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 24, 2023
@alexzielenski alexzielenski changed the title Add verify-examples.sh and make example gen not rely on GOPATH Add verify-examples.sh and switch from makefiles to go test and go generate Aug 24, 2023
@apelisse
Copy link
Member

/lgtm

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

sttts commented Aug 24, 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 24, 2023
@k8s-ci-robot k8s-ci-robot merged commit 3ffd65d into kubernetes:master Aug 24, 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/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants