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

nogo: adjust include/exclude rules #124155

Merged
merged 1 commit into from
May 14, 2024

Conversation

rickystewart
Copy link
Collaborator

As of #79929, we have been using only_files using the pattern cockroach/pkg in order to only run lints against first-party code. While this has worked locally, this doesn't work in remote execution where the structure of the staging directory for the code differs. Instead of trying to allow-list first-party code, we instead deny-list third-party code using the name "external". This works for most third-party dependencies except for the cgo code, which need some special handling.

Separately, it is unfortunate that it's difficult to match exclusively against first-party code (see #124154). I will look into whether we can submit a change upstream to address this.

To avoid future packages accidentally skipping lints due to this change, I added a separate lint check to make sure no Go package names end in the string external.

Epic: CRDB-8308
Release note: None

As of cockroachdb#79929, we have been using `only_files` using the pattern
`cockroach/pkg` in order to only run lints against first-party code.
While this has worked locally, this doesn't work in remote execution
where the structure of the staging directory for the code differs.
Instead of trying to allow-list first-party code, we instead deny-list
third-party code using the name "external". This works for most
third-party dependencies except for the cgo code, which need some
special handling.

Separately, it is unfortunate that it's difficult to match exclusively
against first-party code (see cockroachdb#124154). I will look into whether we can
submit a change upstream to address this.

To avoid future packages accidentally skipping lints due to this change,
I added a separate lint check to make sure no Go package names end in
the string `external`.

Epic: CRDB-8308
Release note: None
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@rickystewart rickystewart requested a review from rail May 14, 2024 18:41
@rickystewart rickystewart marked this pull request as ready for review May 14, 2024 18:41
@rickystewart rickystewart requested a review from a team as a code owner May 14, 2024 18:41
@rickystewart rickystewart added the backport-24.1.x Flags PRs that need to be backported to 24.1. label May 14, 2024
@rickystewart
Copy link
Collaborator Author

bors r=rail

@craig craig bot merged commit 7e83319 into cockroachdb:master May 14, 2024
22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-24.1.x Flags PRs that need to be backported to 24.1.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants