Skip to content
This repository was archived by the owner on Apr 17, 2025. It is now read-only.

Fix Unit tests could not run #253

Closed
wants to merge 1 commit into from
Closed

Fix Unit tests could not run #253

wants to merge 1 commit into from

Conversation

@mochizuki875
Copy link
Contributor

@mochizuki875 mochizuki875 commented Jan 30, 2023 •
edited
Loading

What type of PR is this?:

/kind bug

What this PR does / why we need it:

Fix Makefile to setup envtest.
When make test is executed, download envtest-setup and envtest binaries locally(/hack) if necessary.
This fix references Makefile generated by kubebuilder.

Tested:

make test fails without this fix and passes with it.

Which issue(s) this PR fixes:

#252

@k8s-ci-robot k8s-ci-robot added kind/bug Categorizes issue or PR as related to a bug. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jan 30, 2023
@k8s-ci-robot
Copy link
Contributor

Hi @mochizuki875. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@k8s-ci-robot k8s-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Jan 30, 2023
@mochizuki875
Copy link
Contributor Author

/assign @adrianludwin

adrianludwin
adrianludwin reviewed Feb 10, 2023
View reviewed changes
Copy link
Contributor

@adrianludwin adrianludwin left a comment

Choose a reason for hiding this comment

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

Very cool change, thanks! That's been an annoying part of the setup for a while now.

Just one question before we submit. If you make a change, please squash your commits so there's only one commit in this PR - thanks :)

/ok-to-test

Makefile Outdated
Comment on lines 110 to 113
@echo
@echo "If tests fail due to no matches for kind \"CustomResourceDefinition\" in version \"apiextensions.k8s.io/v1\","
@echo "please remove the old kubebuilder and reinstall it - https://book.kubebuilder.io/quick-start.html#installation"
@echo
Copy link
Contributor

Choose a reason for hiding this comment

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

Can these lines be removed now? I think the makefile now does what this is telling the user to do?

Copy link
Contributor Author

@mochizuki875 mochizuki875 Feb 10, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

@adrianludwin

Can these lines be removed now?

I think so.
Now, kubebuilder is installed as single binary like this(/usr/local/bin/kubebuilder) not like previously.
So these lines may be useless and I removed them.
thx!

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Feb 10, 2023
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: mochizuki875
Once this PR has been reviewed and has the lgtm label, please ask for approval from adrianludwin. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found 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

@mochizuki875 mochizuki875 requested review from adrianludwin and removed request for adrianludwin, srampal and tashimi February 10, 2023 16:24
@adrianludwin
Copy link
Contributor

Actually, there's a better way to do this - I've added setup-envtest to be hermetic as a part of #257. So you can cancel this PR, but thanks for submitting it as I've copied some of the key lines into my PR!

@mochizuki875 mochizuki875 mentioned this pull request Feb 13, 2023
Merged
adrianludwin added a commit to adrianludwin/hierarchical-namespaces that referenced this pull request Feb 15, 2023
@adrianludwin
Update to Go 1.20 and latest KRTE for e2e tests
c930a73 
E2Es broke a few days ago, likely when test-infra upgraded to 1.20. I
also couldn't run staticcheck on my Google workstation, which has 1.20,
with the same failure message. Upgrading staticcheck fixed the problem;
I also took the opportunity to match the test-infra krte image as well
as the Golang version (which caused some minor reformatting), and also
made the integ tests hermetic by including the setup-envtest tool (bug
 kubernetes-retired#252, based on an approach suggested in kubernetes-retired#253).

Tested: couldn't build on my workstation on Go 1.20 before this change;
can after it. Can also run integ tests after deleting local output of
setup-envtest.
adrianludwin added a commit to adrianludwin/hierarchical-namespaces that referenced this pull request Feb 15, 2023
@adrianludwin
Update to Go 1.20 and latest KRTE for e2e tests
7b16f4c 
E2Es broke a few days ago, likely when test-infra upgraded to 1.20. I
also couldn't run staticcheck on my Google workstation, which has 1.20,
with the same failure message. Upgrading staticcheck fixed the problem;
I also took the opportunity to match the test-infra krte image as well
as the Golang version (which caused some minor reformatting), and also
made the integ tests hermetic by including the setup-envtest tool (bug
 kubernetes-retired#252, based on an approach suggested in kubernetes-retired#253).

Tested: couldn't build on my workstation on Go 1.20 before this change;
can after it. Can also run integ tests after deleting local output of
setup-envtest.
@adrianludwin
Copy link
Contributor

This should be fixed now, please lmk if it's not!

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 15, 2023
@k8s-ci-robot
Copy link
Contributor

PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@mochizuki875
Copy link
Contributor Author

@adrianludwin
This PR was fixed in #257 so I'll close this PR.
Thx!

@mochizuki875 mochizuki875 mentioned this pull request Feb 24, 2023
Merged
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer

@adrianludwin adrianludwin adrianludwin left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@adrianludwin adrianludwin

Labels

cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/bug Categorizes issue or PR as related to a bug. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@mochizuki875 @k8s-ci-robot @adrianludwin