chore(ci): Run all tests on the oldest golang version#13405
chore(ci): Run all tests on the oldest golang version#13405bboreham merged 1 commit intoprometheus:mainfrom
Conversation
29d9080 to
3d07a95
Compare
.github/workflows/ci.yml
Outdated
| go: ["1.20", "1.21"] | ||
| container: | ||
| image: quay.io/prometheus/golang-builder:1.21-base | ||
| image: quay.io/prometheus/golang-builder:${{ matrix.go }}-base |
There was a problem hiding this comment.
I think it's simple/ok (for now) to run all the steps for both versions, we can always change if they start diverging.
There was a problem hiding this comment.
I don't like that it runs npm twice. Although they run in parallel, I assume this is chewing up some limited resource.
There was a problem hiding this comment.
I though testing a complete build may make sense. Especially for the part where the UI is embedded. But I don't think we test that after all.
how about running common-build? Or should we just go for tests?
There was a problem hiding this comment.
By the way, the current test runs a complete build:
prometheus/.github/workflows/ci.yml
Line 72 in 34875ae
|
cc @bboreham as discussed. |
|
We need to make sure that the |
e3323e0 to
dcbf959
Compare
38395e1 to
28b2f6f
Compare
|
@bboreham does this look good to you now? @machine424 assuming @bboreham's comments are addressed, merge conflicts need to get resolved, and then we can merge. |
c349d82 to
70c2260
Compare
|
Thanks, done @beorn7 |
.github/workflows/ci.yml
Outdated
| - run: make test | ||
| - run: go test ./tsdb/ -test.tsdb-isolation=false | ||
| - run: go test --tags=stringlabels ./... | ||
| - run: GOARCH=386 go test ./cmd/prometheus |
There was a problem hiding this comment.
This seems to be doing 3 complete sets of tests plus one subset.
That feels like overkill to me; the point is to catch code that has been added which is incompatible with the older Go. Do you think that further incompatibilities will be caught
under different tags or architecture, such that it's worth spending the time?
ISTM that make test would be simplest, but I could also argue that --tags=stringlabels is the mainline version Prometheus releases with.
There was a problem hiding this comment.
If we don't want to take any risks, I'd say yes, we should run the exact go tests than the current version.
- I'd even add the new
go test --tags=dedupelabels ./... - I can set the new GO_ONLY=1 to
make testto only run go tests.
If we're worried about resources, we can make it a daily test, but that may require more human resources...
There was a problem hiding this comment.
We can go with make test now, and enrich if we have a slip out in the future.
Your call ;)
There was a problem hiding this comment.
The most precious resource is developer time. If this test is the slowest part, then that's what everyone has to wait for:
I think 22 minutes is way overkill; please just run one set of tests.
"don't want to take any risks" is not something anyone said about Prometheus.
There was a problem hiding this comment.
Let's go with it then.
(Tell me if I should add the test with -test.tsdb-isolation=false back)
Run test_mixins on the latest golang version Signed-off-by: machine424 <ayoubmrini424@gmail.com>
bboreham
left a comment
There was a problem hiding this comment.
Thanks, this looks fine now.
Run test_mixins on the latest golang version
From #13389 (review)