Skip to content

Use depguard linter to forbid dependencies #20721

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

Closed
wants to merge 1 commit into from
Closed

Use depguard linter to forbid dependencies #20721

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
10 changes: 0 additions & 10 deletions api/go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,13 +26,3 @@ require (
google.golang.org/genproto/googleapis/rpc v0.0.0-20250825161204-c5933d9347a5 // indirect
gopkg.in/yaml.v3 v3.0.1 // indirect
)

// Bad imports are sometimes causing attempts to pull that code.
// This makes the error more explicit.
replace (
go.etcd.io/etcd => ./FORBIDDEN_DEPENDENCY
go.etcd.io/etcd/api/v3 => ./FORBIDDEN_DEPENDENCY
go.etcd.io/etcd/pkg/v3 => ./FORBIDDEN_DEPENDENCY
go.etcd.io/etcd/tests/v3 => ./FORBIDDEN_DEPENDENCY
go.etcd.io/etcd/v3 => ./FORBIDDEN_DEPENDENCY
)
9 changes: 0 additions & 9 deletions client/v3/go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,12 +46,3 @@ replace (
go.etcd.io/etcd/api/v3 => ../../api
go.etcd.io/etcd/client/pkg/v3 => ../pkg
)

// Bad imports are sometimes causing attempts to pull that code.
// This makes the error more explicit.
replace (
go.etcd.io/etcd => ./FORBIDDEN_DEPENDENCY
go.etcd.io/etcd/pkg/v3 => ./FORBIDDEN_DEPENDENCY
go.etcd.io/etcd/v3 => ./FORBIDDEN_DEPENDENCY
go.etcd.io/tests/v3 => ./FORBIDDEN_DEPENDENCY
)
8 changes: 0 additions & 8 deletions etcdctl/go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,11 +54,3 @@ replace (
go.etcd.io/etcd/client/v3 => ../client/v3
go.etcd.io/etcd/pkg/v3 => ../pkg
)

// Bad imports are sometimes causing attempts to pull that code.
// This makes the error more explicit.
replace (
go.etcd.io/etcd => ./FORBIDDEN_DEPENDENCY
go.etcd.io/etcd/v3 => ./FORBIDDEN_DEPENDENCY
go.etcd.io/tests/v3 => ./FORBIDDEN_DEPENDENCY
)
8 changes: 0 additions & 8 deletions etcdutl/go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,14 +12,6 @@ replace (
go.etcd.io/etcd/server/v3 => ../server
)

// Bad imports are sometimes causing attempts to pull that code.
// This makes the error more explicit.
replace (
go.etcd.io/etcd => ./FORBIDDEN_DEPENDENCY
go.etcd.io/etcd/v3 => ./FORBIDDEN_DEPENDENCY
go.etcd.io/tests/v3 => ./FORBIDDEN_DEPENDENCY
)

require (
github.com/coreos/go-semver v0.3.1
github.com/dustin/go-humanize v1.0.1
Expand Down
11 changes: 0 additions & 11 deletions pkg/go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,14 +32,3 @@ require (
)

replace go.etcd.io/etcd/client/pkg/v3 => ../client/pkg

// Bad imports are sometimes causing attempts to pull that code.
// This makes the error more explicit.
// Etcd contains lots of packages and dependency relationship.
// Shouldn't import unnecessary dependencies
replace (
go.etcd.io/etcd => ./FORBIDDEN_DEPENDENCY
go.etcd.io/etcd/api/v3 => ./FORBIDDEN_DEPENDENCY
go.etcd.io/etcd/tests/v3 => ./FORBIDDEN_DEPENDENCY
go.etcd.io/etcd/v3 => ./FORBIDDEN_DEPENDENCY
)
6 changes: 0 additions & 6 deletions server/go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,9 +82,3 @@ replace (
go.etcd.io/etcd/client/v3 => ../client/v3
go.etcd.io/etcd/pkg/v3 => ../pkg
)

// Bad imports are sometimes causing attempts to pull that code.
// This makes the error more explicit.
replace go.etcd.io/etcd => ./FORBIDDEN_DEPENDENCY

replace go.etcd.io/tests/v3 => ./FORBIDDEN_DEPENDENCY
62 changes: 62 additions & 0 deletions tools/.golangci.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ version: "2"
linters:
default: none
enable: # please keep this alphabetized
- depguard # Implement forbidden dependencies
- errorlint
- ineffassign
- nakedret
Expand All @@ -17,6 +18,67 @@ linters:
- usetesting
- whitespace
settings:
depguard:
rules:
api:
list-mode: lax
files:
# Check files inside the api module/directory, but skip the server/etcdserver/api
# directory. Note that this is configured this way as depguard doesn't support relative
# paths.
# > Should always prefix a file glob with **/ as files are matched against absolute
# > paths.
# Refer to: https://github.com/OpenPeeDeeP/depguard?tab=readme-ov-file#config
# Could be simplified if they resolve issue: https://github.com/OpenPeeDeeP/depguard/issues/54
- '**/api/**/*.go'
- '!**/server/etcdserver/api/**/*.go'
Comment on lines +33 to +34
Copy link
Member

Choose a reason for hiding this comment

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

Can we separate configuration for different modules?

I suggest that we define separate rules for each module, following exactly the same way as we do now in each go.mod file.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, we can separate the configuration per module. I initially had it that way, but then it looked very repetitive, and I thought of consolidating etcdutl, etcdtl, and server as they have the same rule.

However, what you're looking at is a depguard limitation. The path needs to be absolute (refer to OpenPeeDeeP/depguard#54).

So, breaking down these two lines:

  1. '**/api/**/*.go': Any go file that is in a path with /api/. Note that this would match any files inside server/etcdserver/api
  2. '!**/server/etcdserver/api/**/*.go': Deny files that have a path like /server/etcdserver/api. Emphasis on the exclamation mark.

You can see the same configuration in other projects. i.e., this is from woodpecker: https://github.com/woodpecker-ci/woodpecker/blob/388557d94aa9fe06841a207eba4ffa4ba0ae5a05/.golangci.yaml#L109-L112

Copy link
Member

Choose a reason for hiding this comment

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

However, what you're looking at is a depguard limitation. The path needs to be absolute (refer to OpenPeeDeeP/depguard#54).

do not get time to deep it for now. May take a close look sometime later.

But my immediate feeling is that it's more complicated & hard to maintain if we mix different modules.

but then it looked very repetitive

repetitive should be a minor concern. It's much much clearer and easy to maintain.

Copy link
Member Author

Choose a reason for hiding this comment

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

Alright, I'll change it, but be aware that we'll still need the negation ! paths due to depguard's limitation with paths.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done, I did split etcdctl, etcdutl, and server, which were sharing the configuration.

Copy link
Member

Choose a reason for hiding this comment

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

Each time a new package is added, we need to verify the depguard settings

How does it differ from the current setup? We need to verify FORBIDDEN_DEPENDENCY clauses, it's as easy to miss. For example we haven't really added FORBIDDEN_DEPENDENCY clauses for cache package https://github.com/etcd-io/etcd/blob/main/cache/go.mod

Copy link
Member

Choose a reason for hiding this comment

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

We need to verify FORBIDDEN_DEPENDENCY clauses, it's as easy to miss.

The replace xxx => FORBIDDEN_DEPENDENCY is added in module level (in go.mod files); it should be stable. But depguard add rules in package & files level, which is less stable (as we may add new packages)

Copy link
Member Author

Choose a reason for hiding this comment

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

Either way (the current approach) or a linter, would require manual intervention when we create new packages. As far as I know, there's nothing that would magically work without us adding the forbidden imports.

Either is also prone to errors. I still can't understand why we're forbidding go.etcd.io/etcd. Because if I understand correctly, that package is v1.

If what we don't like about depguard is how they define the list of files. When I did my research I found that:

  • The revive linter has imports-blocklist. Unfortunately there's no way to specify it per module or per file path (like depguard).
  • There's another linter, gomodguard. It's available through golangci-lint. But it has the same issue as revive.
    • It works at a module level per its documentation

      The linter looks for blocked modules in go.mod and searches for imported packages where the imported packages module is blocked. Indirect modules are not considered.

    • As it doesn't have a way to configure paths we would need to use it stand-alone, we can add a .gomodguard.yaml in each module.
      • Pro: This is very similar to how we currently do it
      • Cons:
        1. Adds a new linter to execute, as it needs to be called outside of golangci-lint (maybe not a con, but a change we need)
        2. This linter wouldn't be possible to call from the top-level module once we integrate the Go workspace.

I'll open a PR with the gomodguard approach.

Copy link
Member

Choose a reason for hiding this comment

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

It seems that we can't define rules for multiple modules in .golangci.yaml. It should be fine. Adding a separate .gomodguard.yaml for each module is fine.

2. This linter wouldn't be possible to call from the top-level module once we integrate the Go workspace.

Not sure I understood this. There is no golang source code in the top-level module, nor any replace xxx => FORBIDDEN_DEPENDENCY directives in https://github.com/etcd-io/etcd/blob/main/go.mod

Anyway, the PR #20748 looks good to me. To double confirm, can it clear the blocker to use go workspace as you mentioned in #20721 (comment)?

Copy link
Member Author

Choose a reason for hiding this comment

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

  1. This linter wouldn't be possible to call from the top-level module once we integrate the Go workspace.

Not sure I understood this. There is no golang source code in the top-level module, nor any replace xxx => FORBIDDEN_DEPENDENCY directives in https://github.com/etcd-io/etcd/blob/main/go.mod

This is a minor inconvenience whenever we have the Go workspace. With the workspace, you can run (once):

$ golangci-lint ./...

From the top-level of the repository.

With this linter configured per module, we'll still need to do

$ cd module && gomodguard ./...

Anyway, the PR #20748 looks good to me. To double confirm, can it clear the blocker to use go workspace as you mentioned in #20721 (comment)?

Yes, that would clear the go.mod replaces blocker.

deny:
- pkg: go.etcd.io/etcd/api/v3$
- pkg: go.etcd.io/etcd/pkg/v3
- pkg: go.etcd.io/etcd/v3
- pkg: go.etcd.io/tests/v3
client:
list-mode: lax
files:
- "**/client/v3/**/*.go"
deny:
- pkg: go.etcd.io/etcd/pkg/v3
- pkg: go.etcd.io/etcd/v3
- pkg: go.etcd.io/tests/v3
etcdctl:
list-mode: lax
files:
- "**/etcdctl/**/*.go"
deny:
- pkg: go.etcd.io/etcd/v3
- pkg: go.etcd.io/tests/v3
etcdutl:
list-mode: lax
files:
- "**/etcdutl/**/*.go"
deny:
- pkg: go.etcd.io/etcd/v3
- pkg: go.etcd.io/tests/v3
server:
list-mode: lax
files:
- "**/server/**/*.go"
deny:
- pkg: go.etcd.io/etcd/v3
- pkg: go.etcd.io/tests/v3
pkg:
list-mode: lax
files:
# Check files inside the pkg module/directory, but skip the pkg directories inside
# client and the tools/rw-heatmaps modules. Note that this is configured this way as
# depguard doesn't support relative paths.
- "**/pkg/**/*.go"
- "!**/client/pkg/**/*.go"
- "!**/tools/rw-heatmaps/pkg/**/*.go"
Comment on lines +75 to +77
Copy link
Member

Choose a reason for hiding this comment

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

suggest not to mixes multiple modules.
ditto as #20721 (comment).

Copy link
Member Author

Choose a reason for hiding this comment

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

Same here. This is not mixing multiple modules, it's targeting only the root pkg, excluding other pkg directories in the repo.

deny:
- pkg: go.etcd.io/etcd/api/v3
- pkg: go.etcd.io/etcd/v3
- pkg: go.etcd.io/tests/v3
nakedret:
# Align with https://github.com/alexkohler/nakedret/blob/v1.0.2/cmd/nakedret/main.go#L10
max-func-lines: 5
Expand Down