-
Notifications
You must be signed in to change notification settings - Fork 64
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
💝 Vendorless codegen via shell scripting #386
💝 Vendorless codegen via shell scripting #386
Conversation
Skipping CI for Draft Pull Request. |
4b5dad1
to
6535957
Compare
/test all |
a151ea5
to
efe1245
Compare
/test all |
298a73d
to
6845b07
Compare
b16312b
to
6adc3c7
Compare
The f103e9a should fix the fluke for good. I've relaxed the check, to not fail when there is additional output (the download lines from failed tests), so it shouldn't fail anymore. |
/test all |
/test unit-tests |
2 similar comments
/test unit-tests |
/test unit-tests |
As evidenced with #386 PR History the f103e9a fixed the flaky test for good. Also, It's verified in real projects, see: #386 (comment) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @cardil , I think it looks great. I left a few comments and questions.
Running unit tests with Golang 1.21 under test/ leaves go.work.sum file modified. I guess it shouldn't happen:
Also, running the tests with Golang 1.22.5 for the first time, the tests fail with: https://gist.github.com/mgencur/01e9fe4aeeac5a41727e8a5c98026c83 |
That's unfortunately true. I couldn't find the reason why the unit tests are removing the I'm going to take another look at this quirk.
That shouldn't happen, for sure. I had some issues in the past on Prow, which also has clear GOPATH. I haven't noticed your problem, though. Neither on GH action, or Prow, or on my own box. I'll take a look at this, and try to reproduce... |
@mgencur can you maybe provide a way to reproduce test failures you've had? I tried, but I couldn't get it to fail. I was removing |
Reproduced by running |
Are you sure about that?! I have a different outcome - the unit tests in the $ chmod -R u+w "$(go env GOMODCACHE)"
$ rm -rf "$(go env GOMODCACHE)"
$ go test -count=1 ./...
ok knative.dev/hack 0.001s
ok knative.dev/hack/cmd/script 0.001s
? knative.dev/hack/pkg/constraints [no test files]
? knative.dev/hack/pkg/retcode [no test files]
? knative.dev/hack/pkg/utest/assert [no test files]
? knative.dev/hack/pkg/utest/require [no test files]
--- FAIL: TestShort (0.00s)
short_test.go:23: Expecting to have short flag set for unit tests
FAIL
FAIL knative.dev/hack/foo 0.001s
ok knative.dev/hack/pkg/inflator/cli 0.001s
ok knative.dev/hack/pkg/inflator/extract 0.001s
ok knative.dev/hack/shell 1.183s
FAIL |
Ok. I was able to reproduce it with the following (see: output): $ chmod -R u+w "$(go env GOMODCACHE)"
$ rm -rf "$(go env GOMODCACHE)"
$ go test -count=1 ./test/... Although that fail doesn't occur when running with I can add it to the prefetch method, but IDK is that worth it?!? Hmm... |
Nice! Everything seems to work now. |
LGTM |
/assign @upodroid @mgencur already reviewed this PR, and #386 (comment) shows PRs to Eventing and Client that pass with this change, both with vendor dir and without it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cardil, upodroid 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 |
/unhold |
Changes
/kind enhancement
Fixes #385
Verified in real projects, see: #386 (comment)
Release Note