🐛(go/v4, deployImage): fix scaffold to allow run the tests directly. Currently, is only possible run via the makefile targets #3365
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
k8s-ci-robot
added
size/L
camilamacedo86
force-pushed
the
fix-run-tests-via-ide
branch
2 times, most recently
from
235bc4a
to
4e05690
Compare
|
/pull-kubebuilder-e2e-k8s-1-24-7 |
|
pull-kubebuilder-e2e-k8s-1-26-0 |
|
/pull-kubebuilder-e2e-k8s-1-26-0 |
|
/pull-kubebuilder-e2e-k8s-1-24-7 |
2 similar comments
|
/pull-kubebuilder-e2e-k8s-1-24-7 |
|
/pull-kubebuilder-e2e-k8s-1-24-7 |
|
/test pull-kubebuilder-e2e-k8s-1-26-0 |
|
/test pull-kubebuilder-e2e-k8s-1-24-7 |
|
/test pull-kubebuilder-e2e-k8s-1-26-0 |
|
One small observation:
|
| // K8SVersion define the k8s version used to do the scaffold | ||
| // so that is possible retrieve the binaries | ||
| K8SVersion string | ||
|
|
There was a problem hiding this comment.
@camilamacedo86 I like this change. But have this question on adding this option. Having k8s version here would not necessarily mean that my project is compatible with that particular version of K8s. It would just mean that we are scaffolding our test suite such that binaries for that version are downloaded. Do we need docs on this, or can we rename this to be K8sTestVersion?
There was a problem hiding this comment.
The binaries downloaded are within the version defined on the Makefile. Users would need to update the K8s version here at the same way that they need to update in the Makefile when then want to change that.
Please, feel free to provide any wording suggestion that you see that would fit better and make things more understandable that I will accepted :-)
There was a problem hiding this comment.
I updated to have the same name used in the Makefile. (ENVTEST_K8S_VERSION)
k8s-ci-robot
added
the
needs-rebase
|
Hi @pratikmota,
How would we be able to change it dynamic? What is your suggestion to do that? |
camilamacedo86
force-pushed
the
fix-run-tests-via-ide
branch
2 times, most recently
from
c88175d
to
d99a712
Compare
k8s-ci-robot
removed
the
needs-rebase
camilamacedo86
force-pushed
the
fix-run-tests-via-ide
branch
from
d99a712
to
6a40d4d
Compare
camilamacedo86
force-pushed
the
fix-run-tests-via-ide
branch
from
6a40d4d
to
c39ad11
Compare
@camilamacedo86 Okay. I just saw similar changes without hardcode version- "BinaryAssetsDirectory: filepath.Join({{ .BaseDirectoryRelativePath }}, "bin", "k8s",fmt.Sprintf("{{ .K8SVersion }}-%%s-%%s", runtime.GOOS, runtime.GOARCH))," |
| // The BinaryAssetsDirectory is only required if you want to run the tests directly | ||
| // without call the makefile target test. If not informed it will look for the | ||
| // default path defined in controller-runtime which is /usr/local/kubebuilder/. | ||
| // Note that you must have the required binaries setup under the bin directory to perform | ||
| // the tests directly. When we run make test it will be setup and used automatically. | ||
| BinaryAssetsDirectory: filepath.Join({{ .BaseDirectoryRelativePath }}, "bin", "k8s", | ||
| fmt.Sprintf("{{ .K8SVersion }}-%%s-%%s", runtime.GOOS, runtime.GOARCH)), | ||
|
|
|
Hi @pratikmota,
It is in this way in the boilerplate/templates see https://github.com/kubernetes-sigs/kubebuilder/pull/3365/files#r1186883942. However, the tool does the scaffolds of the code within the values that exist in the K8SVersion in the specific Kubebuilder. Then, after that, if users want to change their projects they need to update manually as in any other place that we scaffold versions. |
|
@varshaprasad96 all concerns, questions and suggestions shows are addressed now. WDYT? |
|
@Kavinjsir , @varshaprasad96 @everettraven @rashansmith Could we get this one merged? WDYT? |
k8s-ci-robot
added
the
needs-rebase
camilamacedo86
force-pushed
the
fix-run-tests-via-ide
branch
from
c39ad11
to
90c3b1b
Compare
k8s-ci-robot
removed
the
needs-rebase
|
/test pull-kubebuilder-e2e-k8s-1-26-0 |
There was a problem hiding this comment.
Just 2 nits in the comments. Rest looks good to me! Thanks @camilamacedo86
/lgtm
/approve
| @@ -165,6 +171,14 @@ var _ = BeforeSuite(func() { | |||
| testEnv = &envtest.Environment{ | |||
| CRDDirectoryPaths: []string{filepath.Join({{ .CRDDirectoryRelativePath }}, "config", "crd", "bases")}, | |||
| ErrorIfCRDPathMissing: {{ .Resource.HasAPI }}, | |||
|
|
|||
| // The BinaryAssetsDirectory is only required if you want to run the tests directly | |||
| // without call the makefile target test. If not informed it will look for the | |||
There was a problem hiding this comment.
| // without call the makefile target test. If not informed it will look for the | |
| // without calling the makefile target test. If not informed it will look for the |
| // without call the makefile target test. If not informed it will look for the | ||
| // default path defined in controller-runtime which is /usr/local/kubebuilder/. | ||
| // Note that you must have the required binaries setup under the bin directory to perform | ||
| // the tests directly. When we run make test it will be setup and used automatically. |
There was a problem hiding this comment.
Nit ```suggestion
// the tests directly. When we run make test it will be setup using "setup-envtest" and used automatically.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: camilamacedo86, varshaprasad96 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 |
Description
Fix the scaffold to allow running the tests directly. Currently, it will fail because we install the required binaries at the bin directory inside of the project but that is not the default path configured in controller-runtime.