ci: add a PR gate that actually checks the generated code - #548
Merged
Conversation
This repo has had no GitHub Actions at all -- only a .travis.yml, and Travis
stopped serving open-source repos long ago. So PRs here run zero build and zero
tests, which means `mergeable_state: clean` on a codegen PR currently means
"nobody is checking", not "it passed".
What this adds
--------------
`.github/workflows/ci.yml` with three checks and a single `ci-gate` aggregator,
so branch protection only ever needs one required check name regardless of how
the jobs are rearranged later.
The gate keeps only what decides whether the generated SDK is usable:
ci-syntax `go build ./...` + gofmtcheck -- covers services/ (the generated
packages). Also catches duplicate imports and duplicate
declarations, which Go treats as compile errors.
lint `go vet ./...` -- real bug patterns (printf arg mismatch, lock
copying, unused assignments), not style.
test `make test-cov`.
Coverage upload is a continue-on-error step inside `test` and is deliberately
not part of `ci-gate`: an upload credential problem must never turn a PR red.
Also fixes a test that could not pass reliably
----------------------------------------------
`TestClientTimeout` pointed the client at https://httpbin.org/delay/2, a
third-party public service. When that service is unavailable the assertions
fail -- measured: it returned 503 and the test failed. There is no
testing.Short() guard either, so `-short` cannot skip it. `Test_errorHandler`
had a second reference to the same host.
Both now use a local httptest server. The handler also selects on
r.Context().Done() so it returns as soon as the client times out and
srv.Close() does not block.
Removes .travis.yml, which describes a CI that has not run in years.
Verified locally
----------------
make ci-syntax exit 0 (and exit 2 on an injected syntax error,
and exit 2 on injected gofmt drift)
make lint exit 0
make test-cov exit 0 (was failing on TestClientTimeout before this)
Not included: a scheduled compatibility matrix. The declared floor in go.mod is
go 1.13 and I have not verified the current tree builds on it, so shipping that
matrix now would just produce a red nightly. Worth doing as a follow-up.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.




Why
This repo has had no GitHub Actions at all — only a
.travis.yml, and Travis stopped serving open-source repos long ago. So PRs here currently run zero build and zero tests.That matters for the codegen PRs specifically:
mergeable_state: cleanon them right now means "nobody is checking", not "it passed".What this adds
.github/workflows/ci.yml— three checks plus a singleci-gateaggregator, so branch protection only ever needs one required check name no matter how the jobs are rearranged later.The gate keeps only what decides whether the generated SDK is actually usable:
ci-syntaxgo build ./...+gofmtcheckservices/(the generated packages). Also catches duplicate imports and duplicate declarations — Go treats both as compile errorslintgo vet ./...testmake test-covCoverage upload is a
continue-on-errorstep insidetestand is deliberately not inci-gate'sneeds— an upload credential problem must never turn a PR red.Also: a test that could not pass reliably
TestClientTimeoutpointed the client athttps://httpbin.org/delay/2, a third-party public service. When that service is unavailable the assertions fail — measured: it returned 503 and the test failed. There is notesting.Short()guard either, so-shortcannot skip it.Test_errorHandlerhad a second reference to the same host.Both now use a local
httptestserver. The handler also selects onr.Context().Done(), so it returns as soon as the client times out andsrv.Close()does not block..travis.ymlis removed — it describes a CI that has not run in years, and leaving it implies coverage that does not exist.Verified locally
Not included
A scheduled compatibility matrix. The declared floor in
go.modisgo 1.13and I have not verified the current tree builds on it — shipping that matrix now would just produce a red nightly. Worth doing as a follow-up, together with deciding whethergo 1.13is still the floor we want to claim.