client_golang icon indicating copy to clipboard operation
client_golang copied to clipboard

ci: daggerize test and lint pipelines

Open marcosnils opened this issue 1 year ago • 5 comments

:wave: coming from #1436

TL;DR:

  1. Install the dagger CLI (https://docs.dagger.io/quickstart/cli)
  2. Check out this PR and run:
dagger --src . call make-all --args "check_license test"

you can alternatively tell dagger to run the tests against this PR's branch without having to check out your code in your machine as follows:

dagger call -m github.com/prometheus/client_golang@pull/1534/merge --src https://github.com/prometheus/client_golang#pull/1534/merge \
 make-all --args "check_license test"

This runs the same tests the current GHA pipeline runs here https://github.com/prometheus/client_golang/actions/runs/9419021570/workflow#L60 for all the supported_go_versions.txt file.


Hopefully this serves as a starting point to make this project's CI pipelines more reproducible and consistent across different environments :pray: .

Some considerations I took while daggerizing this pipeline:

  • I've decided to "wrap" as much as I could in order to make the change the least disruptive. As you can see from the PR, I kept the same make UX the current repository has
  • I mostly focused on the go.yml (https://github.com/prometheus/client_golang/actions/runs/9419021570/workflow) and golangci-lint.yml (https://github.com/prometheus/client_golang/actions/runs/9419021582/workflow) workflows as those are mostly the ones that get executed for each PR and commit to main.
  • I've noticed that the GHA workflow files seem to updated from the prometheus/prometheus repository. Given that I needed to modify them, I just did it in this repo as I'm unsure which would be the proper way to proceed here.

cc @kakkoyun

marcosnils avatar Jun 07 '24 21:06 marcosnils

you can alternatively tell dagger to run the tests against this PR's branch without having to check out your code in your machine as follows:

dagger -m github.com/prometheus/client_golang@refs/pull/1534/merge call --src [email protected]:prometheus/client_golang#refs/pull/1534/merge make-all --args "check_license test"

For some reason that git URL failed for me, but this worked:

dagger call -m github.com/prometheus/client_golang@pull/1534/merge --src https://github.com/prometheus/client_golang#pull/1534/merge \
 make-all --args "check_license test"

I get some test failures, is that expected @marcosnils ?

[...]
FAIL    github.com/prometheus/client_golang/prometheus  16.423s
ok      github.com/prometheus/client_golang/prometheus/collectors       0.022s
ok      github.com/prometheus/client_golang/prometheus/graphite 0.026s
ok      github.com/prometheus/client_golang/prometheus/internal 0.026s
ok      github.com/prometheus/client_golang/prometheus/promauto 0.026s
ok      github.com/prometheus/client_golang/prometheus/promhttp 0.386s
ok      github.com/prometheus/client_golang/prometheus/push     0.023s
ok      github.com/prometheus/client_golang/prometheus/testutil 0.026s
ok      github.com/prometheus/client_golang/prometheus/testutil/diff    0.023s
ok      github.com/prometheus/client_golang/prometheus/testutil/promlint        0.023s
FAIL
Stderr:
make: *** [Makefile.common:153: common-test] Error

shykes avatar Jun 07 '24 22:06 shykes

For some reason that git URL failed for me, but this worked:

ok, strange.. both seem to work for me. I'll update the original URL to use yours in the original description :pray:

marcosnils avatar Jun 07 '24 23:06 marcosnils

PR body does not contain a valid type of change. Please refer to CONTRIBUTING.md for more information.

github-actions[bot] avatar Jun 07 '24 23:06 github-actions[bot]

I get some test failures, is that expected @marcosnils ?

just :eyes: at this failures and I have the theory that they're related to some flukes due to timing nits in tests: https://github.com/marcosnils/client_golang/blob/e56689d0f2d0bbff8be277c2e4a8dd71c89ce47a/prometheus/summary_test.go?plain=1#L389. Since make-all runs all go tests in parallel, looks like it's causing that tests to match incorrect assertions.

cc @kakkoyun WDYT?

marcosnils avatar Jun 08 '24 00:06 marcosnils

@marcosnils Awesome! Thanks for the kickstart. I'll check this PR in-depth in the upcoming days. And see what we can improve.

kakkoyun avatar Jun 11 '24 07:06 kakkoyun

hey @kakkoyun gentle ping here. LMK if you want me to rebase this PR if you have the time to :eyes: :pray:

marcosnils avatar Jul 15 '24 14:07 marcosnils

@marcosnils, @ArthurSens, and I finally had some time to spare on this. Thanks again for the contribution.

kakkoyun avatar Jul 19 '24 14:07 kakkoyun

@kakkoyun @ArthurSens I've addressed all the comments above. Please let me know if there's anything else that you'd prefer to be changed :pray:

I've also bumped the dagger version to the latest v0.12 and also rebased against main to resolve conflicts.

marcosnils avatar Jul 19 '24 20:07 marcosnils

things (that means slowly migrating to dagger/main from Make right?

This would be the ideal. At the same time, since Dagger is still calling the same underlying make targets, IMO it's a "good enough" trade-off as an initial approach to see how Dagger feel :relaxed:

marcosnils avatar Jul 30 '24 13:07 marcosnils

If we can fix the naming and add a comment to explain the intention, we are golden.

done! the comment was already there :open_hands: (https://github.com/prometheus/client_golang/pull/1534/files#diff-f18b612d33c74a552af8f75da09a36af695ef16d51ef7537bd1e7ef2d3d09685R53)

marcosnils avatar Aug 07 '24 11:08 marcosnils