Skip to content

hack: fix installing the ginkgo#328

Closed
cynepco3hahue wants to merge 1 commit intoopenshift:masterfrom
cynepco3hahue:fix_ginkgo_binary
Closed

hack: fix installing the ginkgo#328
cynepco3hahue wants to merge 1 commit intoopenshift:masterfrom
cynepco3hahue:fix_ginkgo_binary

Conversation

@cynepco3hahue
Copy link

Signed-off-by: Artyom Lukianov alukiano@redhat.com

@jmencak
Copy link
Contributor

jmencak commented Mar 28, 2022

I don't think this will work in environments that are cut off from Internet access like the ART pipeline. The question is whether we need this. I specifically wanted to avoid installing ginkgo in the past to keep things a bit more robust with less cruft, but perhaps it is time for a change. Thoughts?

@cynepco3hahue
Copy link
Author

I don't think this will work in environments that are cut off from Internet access like the ART pipeline. The question is whether we need this. I specifically wanted to avoid installing ginkgo in the past to keep things a bit more robust with less cruft, but perhaps it is time for a change. Thoughts?

@jmencak Thanks for the comment. Does the ART run e2e tests?

@cynepco3hahue
Copy link
Author

I don't think this will work in environments that are cut off from Internet access like the ART pipeline. The question is whether we need this. I specifically wanted to avoid installing ginkgo in the past to keep things a bit more robust with less cruft, but perhaps it is time for a change. Thoughts?

In general, we can do the same as we are doing with other tools, and build it from the vendor directory.

@jmencak
Copy link
Contributor

jmencak commented Mar 28, 2022

I don't think this will work in environments that are cut off from Internet access like the ART pipeline. The question is whether we need this. I specifically wanted to avoid installing ginkgo in the past to keep things a bit more robust with less cruft, but perhaps it is time for a change. Thoughts?

@jmencak Thanks for the comment. Does the ART run e2e tests?

It does not to my knowledge and we are running the nightly tests separately on the nightlies instead. So as I say, I'm open to change and perhaps the worry/current config is unfounded.

@jmencak
Copy link
Contributor

jmencak commented Mar 28, 2022

I don't think this will work in environments that are cut off from Internet access like the ART pipeline. The question is whether we need this. I specifically wanted to avoid installing ginkgo in the past to keep things a bit more robust with less cruft, but perhaps it is time for a change. Thoughts?

In general, we can do the same as we are doing with other tools, and build it from the vendor directory.

It is an option, but what you're doing is probably simpler and will keep the Makefile cleaner. Let's wait for other thoughts. :) I'm not saying what you do in the PR is a no-go.

@jmencak
Copy link
Contributor

jmencak commented Mar 28, 2022

If we do decide to build ginkgo from vendor, then we should also probably change the Makefile to make full use of it when running NTO e2e tests.

echo "Downloading ginkgo tool"
#go install github.com/onsi/ginkgo/ginkgo
go install github.com/onsi/ginkgo/ginkgo@v1.16.5
GOFLAGS='' go install github.com/onsi/ginkgo/ginkgo@v1.16.5
Copy link
Contributor

Choose a reason for hiding this comment

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

could you please add a comment/note in the commit message about what was the problem and why this fix resolves the problem?

Copy link
Author

Choose a reason for hiding this comment

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

Sure will add the comment.

@ffromani
Copy link
Contributor

do we need ginkgo-the-runner or can we just use the ginkgo packages?

@cynepco3hahue
Copy link
Author

do we need ginkgo-the-runner or can we just use the ginkgo packages?

We need the runner.

@ffromani
Copy link
Contributor

do we need ginkgo-the-runner or can we just use the ginkgo packages?

We need the runner.

ok, last question: how costly is to avoid using the runner?

@cynepco3hahue
Copy link
Author

Ok the CI lane looks better now, it still has failures but at least it succeeded to run
For future investigation:

[Fail] [rfe_id:27368][performance] Validation webhook with API version v1alpha1 profile [It] should reject the creation of the profile with overlapping CPUs 
/go/src/github.com/openshift/cluster-node-tuning-operator/test/e2e/pao/functests/1_performance/performance.go:840
[Fail] [rfe_id:27368][performance] Validation webhook with API version v1alpha1 profile [It] should reject the creation of the profile with overlapping CPUs 
/go/src/github.com/openshift/cluster-node-tuning-operator/test/e2e/pao/functests/1_performance/performance.go:841

@cynepco3hahue
Copy link
Author

ok, last question: how costly is to avoid using the runner?

@fromanirh Just to be sure, by this do you mean how costly it is to create a test binary that we can run without ginkgo?

@ffromani
Copy link
Contributor

ok, last question: how costly is to avoid using the runner?

@fromanirh Just to be sure, by this do you mean how costly it is to create a test binary that we can run without ginkgo?

I mean creating a ginkgo entry point like this: https://github.com/openshift-kni/numaresources-operator/blob/main/test/e2e/serial/e2e_serial_test.go which runs all the tests but does not need the ginkgo runner; we can just use go test.

@yanirq
Copy link
Contributor

yanirq commented Mar 28, 2022

@jmencak @cynepco3hahue either approach is fine.
I suggest having a fix that will release other PRs if one approach is more costly and will take more time (which we can do in another PR)

@ffromani
Copy link
Contributor

I'm not against the current approach btw

@cynepco3hahue
Copy link
Author

I mean creating a ginkgo entry point like this: https://github.com/openshift-kni/numaresources-operator/blob/main/test/e2e/serial/e2e_serial_test.go which runs all the tests but does not need the ginkgo runner; we can just use go test.

I think in this case we probably can not use ginkgo flags, like fail fast. We can consider using it in the future, but first, we should be sure that all our tests are stable for 100%.

@ffromani
Copy link
Contributor

I mean creating a ginkgo entry point like this: https://github.com/openshift-kni/numaresources-operator/blob/main/test/e2e/serial/e2e_serial_test.go which runs all the tests but does not need the ginkgo runner; we can just use go test.

I think in this case we probably can not use ginkgo flags, like fail fast. We can consider using it in the future, but first, we should be sure that all our tests are stable for 100%.

we can, but I agree let's get out of the woods first and if this is the quickest way, let's go with it

@jmencak
Copy link
Contributor

jmencak commented Mar 28, 2022

OK, let's wait for ci/prow/e2e-gcp-pao to pass. Then we can squash, merge and cleanup (hopefully...) later.

@yanirq
Copy link
Contributor

yanirq commented Mar 28, 2022

OK, let's wait for ci/prow/e2e-gcp-pao to pass. Then we can squash, merge and cleanup (hopefully...) later.

I would still pass this PR even if e2e-gcp-pao fails on actual tests which will need to be addressed (not on setup phase)

@ffromani
Copy link
Contributor

ok, last question: how costly is to avoid using the runner?

@fromanirh Just to be sure, by this do you mean how costly it is to create a test binary that we can run without ginkgo?

I mean creating a ginkgo entry point like this: https://github.com/openshift-kni/numaresources-operator/blob/main/test/e2e/serial/e2e_serial_test.go which runs all the tests but does not need the ginkgo runner; we can just use go test.

NON BLOCKING PoC here: #329 let's see how it goes

@ffromani
Copy link
Contributor

ok, last question: how costly is to avoid using the runner?

@fromanirh Just to be sure, by this do you mean how costly it is to create a test binary that we can run without ginkgo?

I mean creating a ginkgo entry point like this: https://github.com/openshift-kni/numaresources-operator/blob/main/test/e2e/serial/e2e_serial_test.go which runs all the tests but does not need the ginkgo runner; we can just use go test.

NON BLOCKING PoC here: #329 let's see how it goes

the initial experiments demonstrate this is doable BUT having a 1:1 replacement is not so easy. We would possibly need some changes in how we run the suites, so by all means let's unblock quickly the e2e testing with this approach while #329 matures (or a better alternative emerges)

@jmencak
Copy link
Contributor

jmencak commented Mar 28, 2022

NON BLOCKING PoC here: #329 let's see how it goes

the initial experiments demonstrate this is doable BUT having a 1:1 replacement is not so easy. We would possibly need some changes in how we run the suites, so by all means let's unblock quickly the e2e testing with this approach while #329 matures (or a better alternative emerges)

I do not have a problem with the approach in this PR to get the tests working fast for the time being.

@yanirq
Copy link
Contributor

yanirq commented Mar 28, 2022

/retest

1 similar comment
@cynepco3hahue
Copy link
Author

/retest

@jmencak
Copy link
Contributor

jmencak commented Mar 29, 2022

Great to see the ci/prow/e2e-gcp-pao job succeeded. Can we squash the commits? Then I'm happy to approve.

- fix installation of the ginkgo under run-functests script
- add release annotation to webhook configuration resources

Signed-off-by: Artyom Lukianov <alukiano@redhat.com>
@cynepco3hahue
Copy link
Author

Great to see the ci/prow/e2e-gcp-pao job succeeded. Can we squash the commits? Then I'm happy to approve.

Done.

@jmencak
Copy link
Contributor

jmencak commented Mar 29, 2022

Thank you for a clean commit message.
/approve
/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Mar 29, 2022
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Mar 29, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cynepco3hahue, jmencak

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

The pull request process is described here

Details 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

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 29, 2022
@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-ci openshift-ci bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 29, 2022
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Mar 29, 2022

@cynepco3hahue: PR needs rebase.

Details

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.

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Mar 29, 2022

@cynepco3hahue: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/e2e-upgrade e51f226 link true /test e2e-upgrade
ci/prow/e2e-aws-operator e51f226 link true /test e2e-aws-operator
ci/prow/e2e-aws e51f226 link true /test e2e-aws

Full PR test history. Your PR dashboard.

Details

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. I understand the commands that are listed here.

@cynepco3hahue
Copy link
Author

Fixed under different PR.

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. lgtm Indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

Comments