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

ci: improvements #451

Merged
merged 6 commits into from
Aug 28, 2024
Merged

ci: improvements #451

merged 6 commits into from
Aug 28, 2024

Conversation

jsign
Copy link
Collaborator

@jsign jsign commented Aug 28, 2024

This PR improves the CI configuration to:

  • Remove workflows that now have native support in GH through Settings.
  • Stop using self-hosted for build/lint/test. It isn't properly configured and took ~30min to run for no good reason (i.e: has an ever growing Go cache restoring).
  • The new build/test covers three OS.

Signed-off-by: Ignacio Hagopian <jsign.uy@gmail.com>
Signed-off-by: Ignacio Hagopian <jsign.uy@gmail.com>
Signed-off-by: Ignacio Hagopian <jsign.uy@gmail.com>
Signed-off-by: Ignacio Hagopian <jsign.uy@gmail.com>
Signed-off-by: Ignacio Hagopian <jsign.uy@gmail.com>
- name: Set up Go
uses: actions/setup-go@v5
with:
go-version: 1.22
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Targeting the same version as the minimum required today in go-ethereum.

runs-on: self-hosted
strategy:
matrix:
os: [ubuntu-latest, macos-latest, windows-latest]
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Running tests in three OSs.

with:
go-version: 1.22
- name: Test
run: go test -v -race ./... -timeout=15m
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Looks like the macos-latest is a bit slow, so it requires a bit more time.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Uhm, last run took 7m. But I saw at least one take >10min thus failing.
Let's keep 15m since maybe GH have different machines -- let's avoid potential false positives.

.github/workflows/codeql.yml Outdated Show resolved Hide resolved
@jsign jsign marked this pull request as ready for review August 28, 2024 13:54
@jsign jsign requested a review from gballet August 28, 2024 13:54
Copy link
Member

@gballet gballet left a comment

Choose a reason for hiding this comment

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

Regarding selfhosted, we are currently looking into fixing this problem, which needs to be solved for geth as well. So I don't think there is a need to move away from it, especially since ubuntu-latest is not powerful enough to run all the geth tests.

@@ -33,7 +33,8 @@ import (
"errors"
"fmt"
"io"
mRand "math/rand"
mRandV1 "math/rand"
Copy link
Member

Choose a reason for hiding this comment

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

can we not convert everything to v2?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Unfortuantely no until we can use Go v1.23 which creates an API to do that exactly.

.github/workflows/codeql.yml Outdated Show resolved Hide resolved
@jsign
Copy link
Collaborator Author

jsign commented Aug 28, 2024

Regarding selfhosted, we are currently looking into fixing this problem, which needs to be solved for geth as well. So I don't think there is a need to move away from it, especially since ubuntu-latest is not powerful enough to run all the geth tests.

Why is ubuntu-latest power related to go-verkle? geth tests can still be run with self-hosted if that's useful. What's the benefit of running self-hosted in this repo?

Signed-off-by: Ignacio Hagopian <jsign.uy@gmail.com>
@gballet
Copy link
Member

gballet commented Aug 28, 2024

I guess. Ok, let's do that.

@gballet gballet merged commit beed15b into master Aug 28, 2024
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants