Skip to content
This repository has been archived by the owner on Aug 29, 2023. It is now read-only.

feat: allow configuring custom go-test runners #443

Merged
merged 3 commits into from
Feb 3, 2023
Merged

Conversation

galargh
Copy link
Contributor

@galargh galargh commented Jan 13, 2023

Related to libp2p/go-libp2p#1632 (comment)

Description

This will allow customising runners for go-test workflow by setting repository UCI_GO_TEST_RUNNER_${RUNNER_OS} configuration variable.

I opted for using configuration variable rather than JSON config like we use in other places because I think it'll provide better experience for those wanting to run CI in forks. With configuration variables, the forks want have runner override set up so they will fall back to the hosted runners by default.

Testing

@galargh galargh requested a review from a team as a code owner January 13, 2023 08:53
@galargh galargh requested a review from laurentsenta January 13, 2023 08:53
@galargh
Copy link
Contributor Author

galargh commented Jan 14, 2023

Variables are not passed to workflows that are triggered by a pull request from a fork.

We might want to consider using the JSON config instead. However, that would require an additional job to run before go-test which would determine what runner to use.

I raised an issue with GitHub team about this - https://github.com/orgs/community/discussions/44322. Another option would be to use this solution as-is and get the benefit of configurable runners for PRs from within the origin and wait for GitHub to resolve the issue.

@galargh
Copy link
Contributor Author

galargh commented Feb 1, 2023

Even though https://github.com/orgs/community/discussions/44322 hasn't been picked up by anyone from GitHub yet, I think we should proceed with this solution. It introduces a way for experimentation with self-hosted runners. In this context, it could be viewed as an upside that it wouldn't extend to external contributors' PRs yet. Finally, I don't think we'll be able to prepare anything more sophisticated by next week, which is when we plan the next release.

@galargh galargh mentioned this pull request Feb 1, 2023
10 tasks

Some aspects of Unified CI workflows are configurable through [configuration variables](https://docs.github.com/en/actions/learn-github-actions/variables#creating-configuration-variables-for-a-repository).

You can customise the runner type for `go-test` through `UCI_GO_TEST_RUNNER_UBUNTU`, `UCI_GO_TEST_RUNNER_WINDOWS` and `UCI_GO_TEST_RUNNER_MACOS` configuration variables. This option will be useful for repositories wanting to use more powerful, [PL self-hosted GitHub Actions runners](https://github.com/pl-strflt/tf-aws-gh-runner).
Copy link
Contributor

Choose a reason for hiding this comment

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

Are environment variables case-insensitive?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When you save a var, it's automatically capitalised. Retrieval is case-insensitive.

@marten-seemann
Copy link
Contributor

Even though https://github.com/orgs/community/discussions/44322 hasn't been picked up by anyone from GitHub yet, I think we should proceed with this solution. It introduces a way for experimentation with self-hosted runners. In this context, it could be viewed as an upside that it wouldn't extend to external contributors' PRs yet

@galargh Can you clarify what happens when run from a fork? It would claim that it's a requirement that PRs authored by outside contributors (i.e. on their forks) work, otherwise we won't be able to accept any community contributions.

@@ -11,7 +11,7 @@ jobs:
env:
COVERAGES: ""
SKIP32BIT: false
runs-on: ${{ format('{0}-latest', matrix.os) }}
runs-on: ${{ fromJSON(vars[format('UCI_GO_TEST_RUNNER_{0}', matrix.os)] || format('"{0}-latest"', matrix.os)) }}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Even though https://github.com/orgs/community/discussions/44322 hasn't been picked up by anyone from GitHub yet, I think we should proceed with this solution. It introduces a way for experimentation with self-hosted runners. In this context, it could be viewed as an upside that it wouldn't extend to external contributors' PRs yet

@galargh Can you clarify what happens when run from a fork? It would claim that it's a requirement that PRs authored by outside contributors (i.e. on their forks) work, otherwise we won't be able to accept any community contributions.

Sure! PRs from forks do not have access to vars yet, so, even if the custom runner variable is set, they'll be using a fallback to format('"{0}-latest"', matrix.os) (the default hosted runners).

Copy link
Contributor

@marten-seemann marten-seemann left a comment

Choose a reason for hiding this comment

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

Thank you for the explanations! The reasoning sounds good to me, but obviously we should try this out somewhere before deploying it.

@galargh galargh merged commit b66f621 into next Feb 3, 2023
@galargh galargh deleted the uci-go-test-runner branch February 3, 2023 09:37
@galargh
Copy link
Contributor Author

galargh commented Feb 3, 2023

Thank you for the explanations! The reasoning sounds good to me, but obviously we should try this out somewhere before deploying it.

We do have a couple of repos following next instead of master right now (through ref attribute on target). We'll also use this mechanism to deploy 1.20 to a subset of repos first before going all out.

galargh added a commit that referenced this pull request Feb 8, 2023
* Add option to skip `32-bit` go test (#412)

Introduce an option to configure `go-test` to allow completely skipping
`32-bit` tests.

Fixes #388

* Run at most 1 dispatch job per ref (#414)

* fix: check if git tag returns any results (#415)

* make go generate print the commands it executs (#440)

* use pull_request_target event for release-check workflow (#295)

* Revert "include cross-package coverage in codecov"

* Revert "Revert "include cross-package coverage in codecov""

* Make automerge a reusable workflow (#260)

* move automerge from template to workflows

* make automerge reusable and use it from new automerge template

* pass parent job name to reusable automerge

* check github actions yamls (#272)

* check github actions yamls

* make yaml linter happy about go-test

* mention VS Code YAML extension in the readme

* add info about other YAML checking extensions

* make yaml checker more generic

* use validate-yaml-schema action from mainline (#277)

* upgrade lewagon/wait-on-check-action to v1.1.1 (#278)

* always add a version.json file if it doesn't exist (#281)

* fix go-test runner string

* check if tag already exists in release-check (#287)

* allow specifying custom PATH for 386 arch (#289)

* use pull_request_target event for release-check workflow

* add comment on missing version.json

* chore: revert release checker path trigger change

* chore: add footnote when non-docs files are modified with the release

* fix: prev version calculation

* feat: allow configuring custom go-test runners (#443)

* feat: allow configuring custom go-test runners

* docs: update readme to include info on configuration variables

* feat: allow skipping go-test on certain OSes (#455)

* feat: standarise JSON config reading

* feat: allow skipping go-test on certain OSes

* fix: go-test conditional

* chore: show config after extracting it

* chore: udpate actions and go modules (#458)

* fix: source read-config from next for now

* simplify Go version upgrade procedure (#280)

* chore: simplify Go version upgrade procedure

* chore: add default for the go-version input of release-check

* Update .github/actions/copy-workflow-go/action.yml

* Update configs/README.md

Co-authored-by: Laurent Senta <[email protected]>

---------

Co-authored-by: Laurent Senta <[email protected]>

* Go through all the workflows and clean them up ahead of the next major release (#462)

* chore: clean up deprecated set-output

* chore: do not use substitution inside run

* chore: do not use substitution in if

* chore: skip env var brakets where possible

* fix: env var substitution

* fix: double toJSON

* Update templates/.github/workflows/js-test-and-release.yml

* feat: create gh releases in release-check/releaser workflows (#456)

* feat: create gh releases in release-check/releaser workflows

* fix: fill expr in release check workflow

* fix: add missing gh token in release check

* fix: add missing prev version env var in release check workflow

* fix: release check in release check

* chore: clean up obsolete step from releaser

* fix: step outputs in release workflows

* fix: labels in releaser

* fix: action gh release

* update go version to 1.20.x (#463)

* update go version to 1.20.x

* fix: go 1.20 upgrade

* Revert "fix: go 1.20 upgrade"

This reverts commit ceb72ef.

* clean up where source ref was set to next (#464)

* perform self-review before final release

---------

Co-authored-by: Masih H. Derkani <[email protected]>
Co-authored-by: Marten Seemann <[email protected]>
Co-authored-by: Laurent Senta <[email protected]>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants