Skip to content

Commit 7ec8848

Browse files
helsaawyhelsaawy
authored
Lint common error wrapping issues, update README (#1969)
* Lint common error wrapping issues, update README Enable `errorlint` to catch common issues with wrapping and testing for errors. Wherever possible, switched to using `errors.Is` and `errors.As`. Exceptions: - function is defined in the same package and explicitly returns a know error variable - returns from functions in `io`, `binary`, `context`, `syscall`, `golang.org/x/sys/windows`, or `golang.org/x/sys/unix` that are (relatively) stable in error return value and type - conversion would interact with with `github.com/pkg/errors` - conversion would be non-trivial and require additional testing/validation - specifically, legacy code in `runhcs` and the root of the repo Rename `context` to `ctx` in `pkg\go-runhcs\*.go` to avoid overshadowing `context` package. Update `README.md`: - run markdown formatter (spaces around code blocks and headers, raw link URLS) - add section on linter and go generate (similar to go-winio's) Signed-off-by: Hamza El-Saawy <[email protected]> * PR: hcserrors(+tests), README Signed-off-by: Hamza El-Saawy <[email protected]> --------- Signed-off-by: Hamza El-Saawy <[email protected]>
1 parent 9fb7881 commit 7ec8848

118 files changed

Lines changed: 522 additions & 323 deletions

File tree

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

‎.github/workflows/ci.yml‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -41,7 +41,6 @@ jobs:
4141
--modules-download-mode=readonly
4242
--timeout=10m
4343
--config=${{ github.workspace }}/.golangci.yml
44-
skip-cache: true # even if a cache is found, don't use it
4544
working-directory: ${{ github.workspace }}/${{ matrix.root }}
4645
env:
4746
GOOS: ${{ matrix.goos }}

‎.golangci.yml‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@ linters:
2020
# - typecheck
2121
# - unused
2222

23+
- errorlint # error wrapping (eg, not using `errors.Is`, using `%s` instead of `%w` in `fmt.Errorf`)
2324
- gofmt # whether code was gofmt-ed
2425
- govet # enabled by default, but just to be sure
2526
- nolintlint # ill-formed or insufficient nolint directives
@@ -53,6 +54,12 @@ issues:
5354
text: "^ST1003: should not use underscores in package names$"
5455
source: "^package cri_containerd$"
5556

57+
# don't bother with propper error wrapping in test code
58+
- path: cri-containerd
59+
linters:
60+
- errorlint
61+
text: "non-wrapping format verb for fmt.Errorf"
62+
5663
# This repo has a LOT of generated schema files, operating system bindings, and other
5764
# things that ST1003 from stylecheck won't like (screaming case Windows api constants for example).
5865
# There's also some structs that we *could* change the initialisms to be Go friendly

‎README.md‎

Lines changed: 76 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -9,15 +9,18 @@ It is primarily used in the [Moby](https://github.com/moby/moby) and [Containerd
99
## Building
1010

1111
While this repository can be used as a library of sorts to call the HCS apis, there are a couple binaries built out of the repository as well. The main ones being the Linux guest agent, and an implementation of the [runtime v2 containerd shim api](https://github.com/containerd/containerd/blob/master/runtime/v2/README.md).
12+
1213
### Linux Hyper-V Container Guest Agent
1314

1415
To build the Linux guest agent itself all that's needed is to set your GOOS to "Linux" and build out of ./cmd/gcs.
16+
1517
```powershell
1618
C:\> $env:GOOS="linux"
1719
C:\> go build .\cmd\gcs\
1820
```
1921

2022
or on a Linux machine
23+
2124
```sh
2225
> go build ./cmd/gcs
2326
```
@@ -33,13 +36,15 @@ make all
3336
```
3437

3538
If the build is successful, in the `./out` folder you should see:
39+
3640
```sh
3741
> ls ./out/
3842
delta.tar.gz initrd.img rootfs.tar.gz
3943
```
4044

4145
### Containerd Shim
42-
For info on the Runtime V2 API: https://github.com/containerd/containerd/blob/master/runtime/v2/README.md.
46+
47+
For info on the [Runtime V2 API](https://github.com/containerd/containerd/blob/master/runtime/v2/README.md).
4348

4449
Contrary to the typical Linux architecture of shim -> runc, the runhcs shim is used both to launch and manage the lifetime of containers.
4550

@@ -48,14 +53,17 @@ C:\> $env:GOOS="windows"
4853
C:\> go build .\cmd\containerd-shim-runhcs-v1
4954
```
5055

51-
Then place the binary in the same directory that Containerd is located at in your environment. A default Containerd configuration file can be generated by running:
56+
Then place the binary in the same directory that Containerd is located at in your environment.
57+
A default Containerd configuration file can be generated by running:
58+
5259
```powershell
5360
.\containerd.exe config default | Out-File "C:\Program Files\containerd\config.toml" -Encoding ascii
5461
```
5562

5663
This config file will already have the shim set as the default runtime for cri interactions.
5764

5865
To trial using the shim out with ctr.exe:
66+
5967
```powershell
6068
C:\> ctr.exe run --runtime io.containerd.runhcs.v1 --rm mcr.microsoft.com/windows/nanoserver:2004 windows-test cmd /c "echo Hello World!"
6169
```
@@ -64,16 +72,69 @@ C:\> ctr.exe run --runtime io.containerd.runhcs.v1 --rm mcr.microsoft.com/window
6472

6573
This project welcomes contributions and suggestions. Most contributions require you to agree to a
6674
Contributor License Agreement (CLA) declaring that you have the right to, and actually do, grant us
67-
the rights to use your contribution. For details, visit https://cla.microsoft.com.
75+
the rights to use your contribution. For details, visit [Microsoft CLA](https://cla.microsoft.com).
6876

6977
When you submit a pull request, a CLA-bot will automatically determine whether you need to provide
7078
a CLA and decorate the PR appropriately (e.g., label, comment). Simply follow the instructions
7179
provided by the bot. You will only need to do this once across all repos using our CLA.
7280

73-
We also require that contributors [sign their commits](https://git-scm.com/docs/git-commit) using `git commit -s` or `git commit --signoff` to
74-
certify they either authored the work themselves or otherwise have permission to use it in this project. Please see https://developercertificate.org/ for
75-
more info, as well as to make sure that you can attest to the rules listed. Our CI uses the [DCO Github app](https://github.com/apps/dco) to ensure
76-
that all commits in a given PR are signed-off.
81+
We require that contributors sign their commits
82+
to certify they either authored the work themselves or otherwise have permission to use it in this project.
83+
84+
We also require that contributors sign their commits using using [`git commit --signoff`][git-commit-s]
85+
to certify they either authored the work themselves or otherwise have permission to use it in this project.
86+
A range of commits can be signed off using [`git rebase --signoff`][git-rebase-s].
87+
88+
Please see [the developer certificate](https://developercertificate.org) for more info,
89+
as well as to make sure that you can attest to the rules listed.
90+
Our CI uses the [DCO Github app](https://github.com/apps/dco) to ensure that all commits in a given PR are signed-off.
91+
92+
### Linting
93+
94+
Code must pass a linting stage, which uses [`golangci-lint`][lint].
95+
Since `./test` is a separate Go module, the linter is run from both the root and the
96+
`test` directories. Additionally, the linter is run with `GOOS` set to both `windows` and
97+
`linux`.
98+
99+
The linting settings are stored in [`.golangci.yaml`](./.golangci.yaml), and can be run
100+
automatically with VSCode by adding the following to your workspace or folder settings:
101+
102+
```json
103+
"go.lintTool": "golangci-lint",
104+
"go.lintOnSave": "package",
105+
```
106+
107+
Additional editor [integrations options are also available][lint-ide].
108+
109+
Alternatively, `golangci-lint` can be [installed][lint-install] and run locally:
110+
111+
```shell
112+
# use . or specify a path to only lint a package
113+
# to show all lint errors, use flags "--max-issues-per-linter=0 --max-same-issues=0"
114+
> golangci-lint run
115+
```
116+
117+
To run across the entire repo for both `GOOS=windows` and `linux`:
118+
119+
```powershell
120+
> foreach ( $goos in ('windows', 'linux') ) {
121+
foreach ( $repo in ('.', 'test') ) {
122+
pwsh -Command "cd $repo && go env -w GOOS=$goos && golangci-lint.exe run --verbose"
123+
}
124+
}
125+
```
126+
127+
### Go Generate
128+
129+
The pipeline checks that auto-generated code, via `go generate`, are up to date.
130+
Similar to the [linting stage](#linting), `go generate` is run in both the root and test Go modules.
131+
132+
This can be done via:
133+
134+
```shell
135+
> go generate ./...
136+
> cd test && go generate ./...
137+
```
77138

78139
## Code of Conduct
79140

@@ -83,7 +144,7 @@ contact [[email protected]](mailto:[email protected]) with any additio
83144

84145
## Dependencies
85146

86-
This project requires Golang 1.17 or newer to build.
147+
This project requires Golang 1.18 or newer to build.
87148

88149
For system requirements to run this project, see the Microsoft docs on [Windows Container requirements](https://docs.microsoft.com/en-us/virtualization/windowscontainers/deploy-containers/system-requirements).
89150

@@ -100,3 +161,10 @@ For additional details, see [Report a Computer Security Vulnerability](https://t
100161

101162
---------------
102163
Copyright (c) 2018 Microsoft Corp. All rights reserved.
164+
165+
[lint]: https://golangci-lint.run/
166+
[lint-ide]: https://golangci-lint.run/usage/integrations/#editor-integration
167+
[lint-install]: https://golangci-lint.run/usage/install/#local-installation
168+
169+
[git-commit-s]: https://git-scm.com/docs/git-commit#Documentation/git-commit.txt--s
170+
[git-rebase-s]: https://git-scm.com/docs/git-rebase#Documentation/git-rebase.txt---signoff

‎cmd/containerd-shim-runhcs-v1/delete.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -119,7 +119,7 @@ The delete command will be executed in the container's bundle as its cwd.
119119
if err := winapi.NetUserDel(
120120
"",
121121
username,
122-
); err != nil && err != winapi.NERR_UserNotFound {
122+
); err != nil && !errors.Is(err, winapi.NERR_UserNotFound) {
123123
fmt.Fprintf(os.Stderr, "failed to delete user %q: %v", username, err)
124124
}
125125

‎cmd/containerd-shim-runhcs-v1/exec_wcow_podsandbox_test.go‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -4,13 +4,13 @@ package main
44

55
import (
66
"context"
7+
"errors"
78
"testing"
89
"time"
910

1011
task "github.com/containerd/containerd/api/runtime/task/v2"
1112
containerd_v1_types "github.com/containerd/containerd/api/types/task"
1213
"github.com/containerd/containerd/errdefs"
13-
"github.com/pkg/errors"
1414
)
1515

1616
func verifyWcowPodSandboxExecStatus(t *testing.T, wasStarted bool, es containerd_v1_types.Status, status *task.StateResponse) {
@@ -194,7 +194,7 @@ func Test_newWcowPodSandboxExec_Kill_Created(t *testing.T) {
194194

195195
// Call Kill again
196196
err = wpse.Kill(context.TODO(), 0x0)
197-
if errors.Cause(err) != errdefs.ErrNotFound {
197+
if !errors.Is(err, errdefs.ErrNotFound) {
198198
t.Fatalf("Kill should fail with `ErrNotFound` in the exited state got: %v", err)
199199
}
200200
}
@@ -219,7 +219,7 @@ func Test_newWcowPodSandboxExec_Kill_Started(t *testing.T) {
219219

220220
// Call Kill again
221221
err = wpse.Kill(context.TODO(), 0x0)
222-
if errors.Cause(err) != errdefs.ErrNotFound {
222+
if !errors.Is(err, errdefs.ErrNotFound) {
223223
t.Fatalf("Kill should fail with `ErrNotFound` in the exited state got: %v", err)
224224
}
225225
}

‎cmd/containerd-shim-runhcs-v1/rootfs.go‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -63,6 +63,8 @@ func parseLegacyRootfsMount(m *types.Mount) (string, []string, error) {
6363
if strings.HasPrefix(option, mount.ParentLayerPathsFlag) {
6464
err := json.Unmarshal([]byte(option[len(mount.ParentLayerPathsFlag):]), &parentLayerPaths)
6565
if err != nil {
66+
// TODO (go1.20): use multierror via fmt.Errorf("...: %w; ...: %w", ...)
67+
//nolint:errorlint // non-wrapping format verb for fmt.Errorf
6668
return "", nil, fmt.Errorf("unmarshal parent layer paths from mount: %v: %w", err, errdefs.ErrFailedPrecondition)
6769
}
6870
// Would perhaps be worthwhile to check for unrecognized options and return an error,

‎cmd/containerd-shim-runhcs-v1/service_internal_podshim_test.go‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ package main
44

55
import (
66
"context"
7+
"errors"
78
"fmt"
89
"math/rand"
910
"strconv"
@@ -628,7 +629,7 @@ func Test_PodShim_updateInternal_Error(t *testing.T) {
628629
if err == nil {
629630
t.Fatal("expected to get an error for incorrect resource's type")
630631
}
631-
if err != errNotSupportedResourcesRequest {
632+
if !errors.Is(err, errNotSupportedResourcesRequest) {
632633
t.Fatalf("expected to get errNotSupportedResourcesRequest, instead got %v", err)
633634
}
634635
}

‎cmd/containerd-shim-runhcs-v1/service_internal_taskshim_test.go‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ package main
44

55
import (
66
"context"
7+
"errors"
78
"fmt"
89
"math/rand"
910
"os"
@@ -615,7 +616,7 @@ func Test_TaskShim_updateInternal_Error(t *testing.T) {
615616
if err == nil {
616617
t.Fatal("expected to get an error for incorrect resource's type")
617618
}
618-
if err != errNotSupportedResourcesRequest {
619+
if !errors.Is(err, errNotSupportedResourcesRequest) {
619620
t.Fatalf("expected to get errNotSupportedResourcesRequest, instead got %v", err)
620621
}
621622
}

‎cmd/containerd-shim-runhcs-v1/service_internal_test.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,7 @@ import (
1717

1818
func verifyExpectedError(t *testing.T, resp interface{}, actual, expected error) {
1919
t.Helper()
20-
if actual == nil || errors.Cause(actual) != expected || !errors.Is(actual, expected) {
20+
if actual == nil || errors.Cause(actual) != expected || !errors.Is(actual, expected) { //nolint:errorlint
2121
t.Fatalf("expected error: %v, got: %v", expected, actual)
2222
}
2323

‎cmd/gcstools/generichook.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -43,7 +43,7 @@ func runGenericHook() error {
4343

4444
out, err := hookCmd.CombinedOutput()
4545
if err != nil {
46-
return fmt.Errorf("failed to run nvidia cli tool with: %v, %v", string(out), err)
46+
return fmt.Errorf("failed to run nvidia cli tool with: %v, %w", string(out), err)
4747
}
4848

4949
return nil

0 commit comments

Comments
 (0)