✨ Use ErrorIfCRDPathMissing in EnvTest #1765

jamielennox · 2020-11-02T06:57:26Z

The error message if the path to CRDs is missing is misleading.
envtest.Environment has an option to throw a sensible error here if the
path is wrong and for kubebuilder it would be likely we always want to
upload our CRDs.

k8s-ci-robot · 2020-11-02T06:57:33Z

Welcome @jamielennox!

It looks like this is your first PR to kubernetes-sigs/kubebuilder 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/kubebuilder has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot · 2020-11-02T06:57:34Z

Hi @jamielennox. 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.

pkg/plugin/v2/scaffolds/internal/templates/controller/controller_suitetest.go

pkg/plugin/v3/scaffolds/internal/templates/config/controller/controller_suitetest.go

jamielennox · 2020-11-02T07:50:31Z

I'm not sure how you handle changes here. You can squash the PR or i can merge them and force push. Let me know.

prafull01 · 2020-11-02T07:55:01Z

Hi @jamielennox You need to squash the commits into 1 commit and force push. Thanks

Adirio

Code-wise, the change LGTM. I don't know what is implied by this flag so someone with better knowledge about EnvTest should aprove it.

/lgtm

prafull01 · 2020-11-02T09:08:49Z

It is a good to have functionality, when the CRD path specified envtest is not present, it throws error (which it should be). With current implementation even if the CRD is not present, the scaffolded suite doesn't throw error.
I tested it locally, kb init and kb create api, these two command will scaffold the suite_test.go but does not generate the config/crd/bases folder until I run make test all:

Running tests before make manifests without these changes:

prafull@EMPID18004:~/go/src/github.com/prafull01/kubebuilder/proj$ go test -v ./controllers/...
=== RUN   TestAPIs
Running Suite: Controller Suite
===============================
Random Seed: 1604307472
Will run 0 of 0 specs



Ran 0 of 0 Specs in 4.208 seconds
SUCCESS! -- 0 Passed | 0 Failed | 0 Pending | 0 Skipped
--- PASS: TestAPIs (4.21s)
PASS
ok  	sigs.k8s.io/kubebuilder/controllers	4.218s

Running tests before make manifests with these changes:

prafull@EMPID18004:~/go/src/github.com/prafull01/kubebuilder/proj$ go test -v ./controllers/...
=== RUN   TestAPIs
Running Suite: Controller Suite
===============================
Random Seed: 1604307507
Will run 0 of 0 specs

STEP: bootstrapping test environment
2020-11-02T14:28:27.418+0530	DEBUG	controller-runtime.test-env	starting control plane	{"api server flags": []}
2020-11-02T14:28:32.630+0530	DEBUG	controller-runtime.test-env	installing CRDs
Failure [5.212 seconds]
[BeforeSuite] BeforeSuite 
/home/prafull/go/src/github.com/prafull01/kubebuilder/proj/controllers/suite_test.go:52

  Unexpected error:
      <*os.PathError | 0xc0006711a0>: {
          Op: "stat",
          Path: "../config/crd/bases",
          Err: 0x2,
      }
      stat ../config/crd/bases: no such file or directory
  occurred

  /home/prafull/go/src/github.com/prafull01/kubebuilder/proj/controllers/suite_test.go:63
------------------------------


Ran 0 of 0 Specs in 5.246 seconds
FAIL! -- 0 Passed | 0 Failed | 0 Pending | 0 Skipped

Running test with these changes after make manifests which will generate the CRD:

prafull@EMPID18004:~/go/src/github.com/prafull01/kubebuilder/proj$ go test -v ./controllers/...
=== RUN   TestAPIs
Running Suite: Controller Suite
===============================
Random Seed: 1604307565
Will run 0 of 0 specs



Ran 0 of 0 Specs in 7.440 seconds
SUCCESS! -- 0 Passed | 0 Failed | 0 Pending | 0 Skipped
--- PASS: TestAPIs (7.44s)
PASS
ok  	sigs.k8s.io/kubebuilder/controllers	7.453s

IMO, this change is LGTM.

PTAL for
/cc @camilamacedo86

prafull01 · 2020-11-02T09:11:02Z

/approve

pkg/plugin/v3/scaffolds/internal/templates/config/controller/controller_suitetest.go

camilamacedo86 · 2020-11-02T13:02:24Z

/hold

Until the fix #1770 get merged and this one realised with master.

camilamacedo86 · 2020-11-03T20:14:26Z

HI @jamielennox,

Could you please rebase it with the master, run make generate and push?

/hold
cancel

jamielennox · 2020-11-04T07:48:11Z

I've very simply rebased this, reading #1770 maybe we want to only set true on WireResource?

Adirio · 2020-11-04T08:11:31Z

I've very simply rebased this, reading #1770 maybe we want to only set true on WireResource?

Probably yes

camilamacedo86 · 2020-11-18T18:18:20Z

Hi @jamielennox,

Could you check what will be the behaviour faced after this change in the scenario #1765 (comment)? Could you please let us know?

camilamacedo86 · 2020-12-03T01:27:31Z

Hi @jamielennox,

Could you please get it rebased with the master for we are able to move forward with it?

pkg/plugin/v3/scaffolds/internal/templates/controllers/controller_suitetest.go

pkg/plugins/golang/v3/scaffolds/internal/templates/api/webhook_suitetest.go

The error message if the path to CRDs is missing is misleading. envtest.Environment has an option to throw a sensible error here if the path is wrong and for kubebuilder it would be likely we always want to upload our CRDs.

camilamacedo86

/approve

camilamacedo86 · 2021-01-20T19:52:39Z

/test pull-kubebuilder-e2e-k8s-1-16-15

camilamacedo86 · 2021-01-20T19:53:55Z

/restest

camilamacedo86

It receives approval of 2 very active contributors/maintainers (@prafull01 and @Adirio)
The changes here shows fine for me as well.

Great job @jamielennox 🥇

/lgtm

k8s-ci-robot · 2021-01-20T19:56:24Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Adirio, camilamacedo86, jamielennox, prafull01

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:

~~OWNERS~~ [camilamacedo86]

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

camilamacedo86 · 2021-01-20T20:07:08Z

/lgtm

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Nov 2, 2020

k8s-ci-robot requested review from Adirio and pwittrock November 2, 2020 06:57

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Nov 2, 2020

k8s-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Nov 2, 2020

prafull01 suggested changes Nov 2, 2020

View reviewed changes

pkg/plugin/v2/scaffolds/internal/templates/controller/controller_suitetest.go Outdated Show resolved Hide resolved

Adirio suggested changes Nov 2, 2020

View reviewed changes

pkg/plugin/v3/scaffolds/internal/templates/config/controller/controller_suitetest.go Outdated Show resolved Hide resolved

k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Nov 2, 2020

jamielennox force-pushed the error-if-missing branch from d1bf808 to ee14f79 Compare November 2, 2020 07:57

Adirio approved these changes Nov 2, 2020

View reviewed changes

k8s-ci-robot assigned Adirio Nov 2, 2020

k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. 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 Nov 2, 2020

k8s-ci-robot requested a review from camilamacedo86 November 2, 2020 09:08

camilamacedo86 reviewed Nov 2, 2020

View reviewed changes

pkg/plugin/v3/scaffolds/internal/templates/config/controller/controller_suitetest.go Outdated Show resolved Hide resolved

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 2, 2020

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 3, 2020

jamielennox force-pushed the error-if-missing branch from ee14f79 to 6db8400 Compare November 4, 2020 07:45

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 4, 2020

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 5, 2020

k8s-ci-robot unassigned Adirio Nov 5, 2020

jamielennox force-pushed the error-if-missing branch from 6db8400 to 33f9e41 Compare November 7, 2020 13:35

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 7, 2020

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 18, 2020

camilamacedo86 reviewed Dec 3, 2020

View reviewed changes

pkg/plugin/v3/scaffolds/internal/templates/controllers/controller_suitetest.go Outdated Show resolved Hide resolved

jamielennox force-pushed the error-if-missing branch from 33f9e41 to be655b3 Compare December 14, 2020 09:52

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Dec 14, 2020

camilamacedo86 reviewed Dec 14, 2020

View reviewed changes

pkg/plugins/golang/v3/scaffolds/internal/templates/api/webhook_suitetest.go Outdated Show resolved Hide resolved

jamielennox force-pushed the error-if-missing branch from be655b3 to 2813b8f Compare December 17, 2020 03:44

✨ Use ErrorIfCRDPathMissing in EnvTest

0a5c604

The error message if the path to CRDs is missing is misleading. envtest.Environment has an option to throw a sensible error here if the path is wrong and for kubebuilder it would be likely we always want to upload our CRDs.

jamielennox force-pushed the error-if-missing branch from 2813b8f to 0a5c604 Compare December 17, 2020 04:10

camilamacedo86 approved these changes Dec 17, 2020

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 17, 2020

camilamacedo86 approved these changes Jan 20, 2021

View reviewed changes

k8s-ci-robot assigned camilamacedo86 Jan 20, 2021

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 20, 2021

k8s-ci-robot merged commit 79ecd51 into kubernetes-sigs:master Jan 20, 2021

camilamacedo86 mentioned this pull request Feb 18, 2021

New Release - proposing 3.0.0 stable #2029

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨ Use ErrorIfCRDPathMissing in EnvTest #1765

✨ Use ErrorIfCRDPathMissing in EnvTest #1765

jamielennox commented Nov 2, 2020

k8s-ci-robot commented Nov 2, 2020

k8s-ci-robot commented Nov 2, 2020

jamielennox commented Nov 2, 2020

prafull01 commented Nov 2, 2020

Adirio left a comment

prafull01 commented Nov 2, 2020

prafull01 commented Nov 2, 2020

camilamacedo86 commented Nov 2, 2020

camilamacedo86 commented Nov 3, 2020

jamielennox commented Nov 4, 2020

Adirio commented Nov 4, 2020

camilamacedo86 commented Nov 18, 2020

camilamacedo86 commented Dec 3, 2020

camilamacedo86 left a comment

camilamacedo86 commented Jan 20, 2021

camilamacedo86 commented Jan 20, 2021

camilamacedo86 left a comment •

edited

Loading

k8s-ci-robot commented Jan 20, 2021

camilamacedo86 commented Jan 20, 2021

✨ Use ErrorIfCRDPathMissing in EnvTest #1765

✨ Use ErrorIfCRDPathMissing in EnvTest #1765

Conversation

jamielennox commented Nov 2, 2020

k8s-ci-robot commented Nov 2, 2020

k8s-ci-robot commented Nov 2, 2020

jamielennox commented Nov 2, 2020

prafull01 commented Nov 2, 2020

Adirio left a comment

Choose a reason for hiding this comment

prafull01 commented Nov 2, 2020

prafull01 commented Nov 2, 2020

camilamacedo86 commented Nov 2, 2020

camilamacedo86 commented Nov 3, 2020

jamielennox commented Nov 4, 2020

Adirio commented Nov 4, 2020

camilamacedo86 commented Nov 18, 2020

camilamacedo86 commented Dec 3, 2020

camilamacedo86 left a comment

Choose a reason for hiding this comment

camilamacedo86 commented Jan 20, 2021

camilamacedo86 commented Jan 20, 2021

camilamacedo86 left a comment • edited Loading

Choose a reason for hiding this comment

k8s-ci-robot commented Jan 20, 2021

camilamacedo86 commented Jan 20, 2021

camilamacedo86 left a comment •

edited

Loading