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.

Sign up for GitHub

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

Run crosslink before checking for clean work tree #2623

Closed
wants to merge 3 commits into from
Closed

Run crosslink before checking for clean work tree #2623

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e220da9
run crosslink before checking for clean work tree
dmathieu Feb 17, 2022
1e17000
add crosslink to the ci command
dmathieu Feb 17, 2022
d495bd0
move crosslink to the build task
dmathieu Feb 17, 2022
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
2 changes: 1 addition & 1 deletion .github/workflows/ci.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

My first commit added that as its own step. Though that seemed a bit overkill, I'm happy to bring it back.

Copy link
Contributor

Choose a reason for hiding this comment

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

crosslink is a dependency of lint there is no need to add this.

- name: Build
run: make build
- name: Check clean repository
Expand Down
2 changes: 1 addition & 1 deletion Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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/%)
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

build/%: DIR=$*
build/%:
@echo "$(GO) build $(DIR)/..." \
Expand Down