Skip to content
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

Add CI checks for Go code. #17066

Merged
merged 5 commits into from Feb 27, 2024
Merged

Add CI checks for Go code. #17066

merged 5 commits into from Feb 27, 2024

Conversation

Ferroin
Copy link
Member

@Ferroin Ferroin commented Feb 26, 2024

Summary
  • Adds a GHA workflow which will run go test, go fmt, and go vet for each Go module located under src/go, auto-detecting the paths to the modules and the required version of Go.
Test Plan

CI passes on this PR.

@Ferroin Ferroin force-pushed the golang-tests branch 4 times, most recently from 2c9956b to d413a87 Compare February 26, 2024 17:53
This adds a GHA workflow which will run `go test`, `go fmt`, and `go
vet` for each Go module located under `src/go`, auto-detecting the paths
to the modules and the required version of Go.
@ilyam8
Copy link
Member

ilyam8 commented Feb 27, 2024

Looks needlessly complex to me: I suggest implementing it like it was in go.d.plugin repo:

  • One runner for all checks, adding a separate runner for each "check" is not needed. No pros, only cons. What was the reasoning for separate runners?
  • I see no need in parsing go.mod files. You can add it when/if needed. Please do it the same way it was in the go.d.plugin repo.
  • Also, I need the same steps I have in the go.d.plugin repo: build, vet, test formatting, do tests - no need to improvise.

@Ferroin
Copy link
Member Author

Ferroin commented Feb 27, 2024

Looks needlessly complex to me: I suggest implementing it like it was in go.d.plugin repo:

  • One runner for all checks, adding a separate runner for each "check" is not needed. No pros, only cons. What was the reasoning for separate runners?

This approach gives us:

  • Faster feedback on the individual parts being tested.
  • Precise feedback on what failed without having to look into logs.
  • The ability to confirm that the code passes the test suite independently of it’s formatting.
  • The ability to make only part of the checks mandatory if we need to for some reason (say we know the test suite is broken, this lets us still enforce code formatting as mandatory until the test suite gets fixed without blocking PRs because of the broken test suite).
  • Faster overall run times, and less total time tying up runners.
  • I see no need in parsing go.mod files. You can add it when/if needed. Please do it the same way it was in the go.d.plugin repo.

Handling things this way gets us a few benefits:

  • No need to hard-code what Go modules in our repo to run tests on. When we inevitably add a new one, it will just be picked up by CI automatically without us having to do anything.
  • No need to hard-code what version of Go to use. We can’t readily automate updating any kind of hard-coded version for this, and it’s something that needs to stay up to date.
  • Keying off of the version of Go actually needed by the modules instead of hard-coding it also ensures that we actually continue to work correctly on the version of Go we declare ourselves as needing.
  • Also, I need the same steps I have in the go.d.plugin repo: build, vet, test formatting, do tests - no need to improvise.

The build part is not yet done, all the other checks should be functionally identical to the stuff that was in the Go repo (with some apparent issue with the formatting check that I need to look into). Unless of course there are interdependencies between the steps, which would be really questionable for multiple reasons.

Copy link
Member

@ilyam8 ilyam8 left a comment

Choose a reason for hiding this comment

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

please address #17066 (comment). Needlessly complicated as it is now.

@Ferroin Ferroin force-pushed the golang-tests branch 4 times, most recently from 3239582 to 8eeb9b2 Compare February 27, 2024 16:45
Co-authored-by: Ilya Mashchenko <ilya@netdata.cloud>
@ilyam8
Copy link
Member

ilyam8 commented Feb 27, 2024

Failing tests are expected and don't indicate a problem. The issue is #16997, not everything was copied. Missing testdata will be added in #17064.

@Ferroin Ferroin marked this pull request as ready for review February 27, 2024 18:09
@Ferroin
Copy link
Member Author

Ferroin commented Feb 27, 2024

Given the discussions in the sync today, there is little value for local testing in having a global build target to build all the Go components, and the possibility of having testing that the CMake tooling around our Go components works correctly as part of the regular CI is highly contentious (I still contend that this is something we should be testing, and that the overhead will not be problematic), so I will plan to add that in a separate PR at this point in the interest of not delaying this one any further.

@ilyam8 ilyam8 merged commit da5d8ff into netdata:master Feb 27, 2024
131 of 135 checks passed
ilyam8 added a commit to ilyam8/netdata that referenced this pull request Feb 28, 2024
Co-authored-by: Ilya Mashchenko <ilya@netdata.cloud>
@Ferroin Ferroin deleted the golang-tests branch February 28, 2024 11:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants