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

chore(ci): fix breaking tests #8413

Merged
merged 7 commits into from
Nov 4, 2022
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions .github/workflows/ci-dgraph-load-tests.yml
Original file line number Diff line number Diff line change
Expand Up @@ -16,13 +16,18 @@ on:
jobs:
dgraph-load-tests:
if: github.event.pull_request.draft == false
runs-on: self-hosted
runs-on: [self-hosted, x64]
steps:
- uses: actions/checkout@v3
- name: Get Go Version
run: |
#!/bin/bash
GOVERSION=$({ [ -f .go-version ] && cat .go-version; })
echo "GOVERSION=$GOVERSION" >> $GITHUB_ENV
Copy link
Contributor

Choose a reason for hiding this comment

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

+1 ^ thanks for adding this, could you also do this for go-lint?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@skrdgraph Sure, previously the linter yaml was skipping the Go installation step entirely with skip-go-installation: true . I just pushed changes that model the linter after the Badger linter setup (hence the version bumps). Let me know if that looks okay.

- name: Set up Go
uses: actions/setup-go@v3
with:
go-version: 1.18
go-version: ${{ env.GOVERSION }}
- name: Set up Node
uses: actions/setup-node@v3
with:
Expand Down
9 changes: 7 additions & 2 deletions .github/workflows/ci-dgraph-tests.yml
Original file line number Diff line number Diff line change
Expand Up @@ -16,13 +16,18 @@ on:
jobs:
dgraph-tests:
if: github.event.pull_request.draft == false
runs-on: self-hosted
runs-on: [self-hosted, x64]
steps:
- uses: actions/checkout@v3
- name: Get Go Version
run: |
#!/bin/bash
GOVERSION=$({ [ -f .go-version ] && cat .go-version; })
echo "GOVERSION=$GOVERSION" >> $GITHUB_ENV
- name: Set up Go
uses: actions/setup-go@v3
with:
go-version: 1.18
go-version: ${{ env.GOVERSION }}
- name: Set up Node
uses: actions/setup-node@v3
with:
Expand Down
16 changes: 12 additions & 4 deletions .github/workflows/ci-golang-lint.yml
Original file line number Diff line number Diff line change
Expand Up @@ -19,15 +19,23 @@ jobs:
name: lint
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v2
- uses: actions/checkout@v3
- name: Get Go Version
run: |
#!/bin/bash
GOVERSION=$({ [ -f .go-version ] && cat .go-version; })
echo "GOVERSION=$GOVERSION" >> $GITHUB_ENV
- name: Setup Go
uses: actions/setup-go@v3
with:
go-version: ${{ env.GOVERSION }}
- name: golang-lint
env:
# prevent OOM
GOGC: 10
uses: golangci/golangci-lint-action@v2
uses: golangci/golangci-lint-action@v3.2.0
Copy link
Contributor

Choose a reason for hiding this comment

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

should we just do @latest here?

Copy link
Contributor Author

@joshua-goldstein joshua-goldstein Nov 4, 2022

Choose a reason for hiding this comment

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

Latest or perhaps @ v3 ? That way a major version upgrade wouldn't break the CI

Copy link
Contributor

Choose a reason for hiding this comment

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

if it does break, we would know something changed in the action side. Times when this would break is when the underlying schema would change things the way this action is invoked or some new fields coming in.

Irrespective it's good to do latest here as well IMO

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense! I pinned it to @ latest

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Interesting, @ latest is not possible (see the failed run here). But @ v3 works and will pull the latest minor versions

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

you can probably just do golangci/golangci-lint-action without a version, this is still a guess

with:
# Required: the version of golangci-lint is required and must be specified without patch version: we always use the latest patch version.
version: v1.36
version: v1.48
Copy link
Contributor

Choose a reason for hiding this comment

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

also should we just do latest here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

latest can work here - currently latest is v1.50.1 so not a big bump

Copy link
Contributor

Choose a reason for hiding this comment

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

it's easier if we pin to latest. We shouldn't be doing latest pins on core code though.

The other thing is there may be some go version <> go-lang-ci version compatibility issues.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also pinned to latest. 👍

only-new-issues: true
args: --timeout=10m
skip-go-installation: true