Run crosslink before checking for clean work tree #2623
Conversation
dmathieu
requested review from
jmacd,
MrAlias,
Aneurysm9,
evantorrie,
XSAM,
dashpole,
paivagustavo,
MadVikingGod and
pellared
as code owners
.github/workflows/ci.yml
Outdated
| @@ -41,6 +41,8 @@ jobs: | |||
| run: make dependabot-check license-check lint vanity-import-check | |||
| - name: Build | |||
| run: make build | |||
| - name: Run code generators | |||
| run: make crosslink | |||
There was a problem hiding this comment.
How about also adding it here https://github.com/open-telemetry/opentelemetry-go/blob/main/Makefile#L29?
Side note: Any reason why the GH workflow is not simply executing make ci?
There was a problem hiding this comment.
It seems make ci sets things up and runs the tests, when this part of the workflow only sets things up, and they later run in separate steps?
| @@ -38,7 +38,7 @@ jobs: | |||
| path: ~/.tools | |||
| key: ${{ runner.os }}-${{ env.cache-name }}-${{ hashFiles('./internal/tools/**') }} | |||
| - name: Run linters | |||
| run: make dependabot-check license-check lint vanity-import-check | |||
| run: make dependabot-check license-check lint vanity-import-check crosslink | |||
There was a problem hiding this comment.
I get why this might be added here, but I want to point out that crosslink makes changes to files unlike all the other linters here. So if it does make a change, the error will be in the Check clean repository step. Which can make debugging this more difficult.
There was a problem hiding this comment.
My first commit added that as its own step. Though that seemed a bit overkill, I'm happy to bring it back.
Makefile
Outdated
| @@ -26,7 +26,7 @@ TIMEOUT = 60 | |||
|
|
|||
| .PHONY: precommit ci | |||
| precommit: license-check misspell go-mod-tidy golangci-lint-fix test-default | |||
| ci: dependabot-check license-check lint vanity-import-check build test-default check-clean-work-tree test-coverage | |||
| ci: dependabot-check license-check lint vanity-import-check crosslink build test-default check-clean-work-tree test-coverage | |||
There was a problem hiding this comment.
I think this should probably just be part of the build target instead. Like, generate the proper replaces should exist before trying to build.
| @@ -38,7 +38,7 @@ jobs: | |||
| path: ~/.tools | |||
| key: ${{ runner.os }}-${{ env.cache-name }}-${{ hashFiles('./internal/tools/**') }} | |||
| - name: Run linters | |||
| run: make dependabot-check license-check lint vanity-import-check | |||
| run: make dependabot-check license-check lint vanity-import-check crosslink | |||
There was a problem hiding this comment.
crosslink is a dependency of lint there is no need to add this.
| @@ -79,7 +79,7 @@ generate/%: | $(STRINGER) $(PORTO) | |||
| && cd $(DIR) \ | |||
| && PATH="$(TOOLS):$${PATH}" $(GO) generate ./... && $(PORTO) -w . | |||
|
|
|||
| build: generate $(OTEL_GO_MOD_DIRS:%=build/%) $(OTEL_GO_MOD_DIRS:%=build-tests/%) | |||
| build: crosslink generate $(OTEL_GO_MOD_DIRS:%=build/%) $(OTEL_GO_MOD_DIRS:%=build-tests/%) | |||
There was a problem hiding this comment.
Separating the build from module linting is the current design of this Makefile. These targets are joined together in the precommit and ci targets. I prefer to keep that design.
This goes alongside my suggestion in #2613 (comment)
It runs
make crosslinkbefore checking for a clean work tree, so anything modifyinggo.modin an invalid way would trigger a test failure.This shouldn't need a changelog entry.