From 6e9e34af2eeebbf5490a89b28ce3ec611e4d4efd Mon Sep 17 00:00:00 2001 From: Arjun Raja Yogidas Date: Thu, 5 Sep 2024 20:49:31 +0000 Subject: [PATCH 1/2] add lint checks to CI Signed-off-by: Arjun Raja Yogidas --- .github/ISSUE_TEMPLATE/config.yaml | 2 +- .github/dependabot.yaml | 28 ++--- .github/workflows/ci.yaml | 146 ++++++++++++---------- .golangci.yaml | 14 ++- api/handlers/container/attach.go | 5 +- api/handlers/container/attach_test.go | 6 +- api/handlers/container/create.go | 3 +- api/handlers/container/list_test.go | 2 +- api/handlers/container/logs_test.go | 4 +- api/handlers/exec/start_test.go | 2 +- api/handlers/image/push_test.go | 2 +- api/handlers/network/network_test.go | 4 +- e2e/tests/tests.go | 4 +- go.mod | 4 +- internal/service/container/attach_test.go | 4 +- internal/service/container/logs_test.go | 4 +- internal/service/image/load_test.go | 2 +- internal/service/network/list_test.go | 2 +- setup-test-env.sh | 2 +- 19 files changed, 129 insertions(+), 111 deletions(-) diff --git a/.github/ISSUE_TEMPLATE/config.yaml b/.github/ISSUE_TEMPLATE/config.yaml index 9fb406d..ba822f0 100644 --- a/.github/ISSUE_TEMPLATE/config.yaml +++ b/.github/ISSUE_TEMPLATE/config.yaml @@ -1,5 +1,5 @@ blank_issues_enabled: false contact_links: - - name: Ask a question + - name: Ask a question url: https://github.com/runfinch/finch-daemon/discussions about: Use GitHub Discussions to ask questions, discuss options, or propose new ideas diff --git a/.github/dependabot.yaml b/.github/dependabot.yaml index e22ac0e..888a05e 100644 --- a/.github/dependabot.yaml +++ b/.github/dependabot.yaml @@ -1,16 +1,16 @@ version: 2 updates: -- package-ecosystem: "gomod" - directory: "/" - schedule: - interval: "daily" - commit-message: - prefix: "build" - include: "scope" -- package-ecosystem: "github-actions" - directory: "/" - schedule: - interval: "daily" - commit-message: - prefix: "ci" - include: "scope" + - package-ecosystem: "gomod" + directory: "/" + schedule: + interval: "daily" + commit-message: + prefix: "build" + include: "scope" + - package-ecosystem: "github-actions" + directory: "/" + schedule: + interval: "daily" + commit-message: + prefix: "ci" + include: "scope" diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index 5b33671..c22bd2c 100644 --- a/.github/workflows/ci.yaml +++ b/.github/workflows/ci.yaml @@ -1,68 +1,86 @@ name: CI on: - push: - branches: - - main - paths-ignore: - - '**.md' - pull_request: - branches: - - main - paths-ignore: - - '**.md' - workflow_dispatch: + push: + branches: + - main + paths-ignore: + - '**.md' + pull_request: + branches: + - main + paths-ignore: + - '**.md' + workflow_dispatch: env: - GO_VERSION: '1.22.6' - + GO_VERSION: '1.23.0' + GOLANGCI_LINT_VERSION: '1.60.3' jobs: - git-secrets: - runs-on: ubuntu-latest - steps: - - name: Pull latest awslabs/git-secrets repo - uses: actions/checkout@692973e3d937129bcbf40652eb9f2f61becf3332 # v4.1.7 - with: - repository: awslabs/git-secrets - ref: 1.3.0 - fetch-tags: true - path: git-secrets - - name: Install git secrets from source - run: sudo make install - working-directory: git-secrets - - uses: actions/checkout@692973e3d937129bcbf40652eb9f2f61becf3332 # v4.1.7 - - name: Scan repository for git secrets - run: | - git secrets --register-aws - git secrets --scan-history - unit-test: - runs-on: ubuntu-latest - steps: - - uses: actions/setup-go@0a12ed9d6a96ab950c8f026ed9f722fe0da7ef32 # v5.0.2 - with: - go-version: ${{ env.GO_VERSION }} - cache: false - - name: Checkout finch-deamon repo - uses: actions/checkout@692973e3d937129bcbf40652eb9f2f61becf3332 # v4.1.7 - - name: Build and run unit tests - run: | - make build - make run-unit-tests - e2e-test: - runs-on: ubuntu-latest - steps: - - uses: actions/setup-go@0a12ed9d6a96ab950c8f026ed9f722fe0da7ef32 # v5.0.2 - with: - go-version: ${{ env.GO_VERSION }} - - name: Checkout finch-daemon repo - uses: actions/checkout@692973e3d937129bcbf40652eb9f2f61becf3332 # v4.1.7 - - name: Install Dependencies for e2e Testing - run: ./setup-test-env.sh - - name: Build the daemon - run: make build - - name: Remove default podman network config - run: | - sudo ls /etc/cni/net.d - sudo rm /etc/cni/net.d/87-podman-bridge.conflist - - name: Start finch-daemon - run: sudo bin/finch-daemon --debug --socket-owner $UID & - - name: Run e2e test - run: sudo make run-e2e-tests + git-secrets: + runs-on: ubuntu-latest + steps: + - name: Pull latest awslabs/git-secrets repo + uses: actions/checkout@692973e3d937129bcbf40652eb9f2f61becf3332 # v4.1.7 + with: + repository: awslabs/git-secrets + ref: 1.3.0 + fetch-tags: true + path: git-secrets + - name: Install git secrets from source + run: sudo make install + working-directory: git-secrets + - uses: actions/checkout@692973e3d937129bcbf40652eb9f2f61becf3332 # v4.1.7 + - name: Scan repository for git secrets + run: | + git secrets --register-aws + git secrets --scan-history + lint: + strategy: + matrix: + working_dir: ['.'] + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@692973e3d937129bcbf40652eb9f2f61becf3332 # v4.1.7 + - uses: golangci/golangci-lint-action@v6 # v6.1.0 + with: + version: v${{ env.GOLANGCI_LINT_VERSION }} + working-directory: ${{ matrix.working_dir }} + args: --fix=false --timeout=5m + yamllint: + name: yamllint-lint + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@692973e3d937129bcbf40652eb9f2f61becf3332 # v4.1.7 + - run: yamllint . + unit-test: + runs-on: ubuntu-latest + steps: + - uses: actions/setup-go@0a12ed9d6a96ab950c8f026ed9f722fe0da7ef32 # v5.0.2 + with: + go-version: ${{ env.GO_VERSION }} + cache: false + - name: Checkout finch-deamon repo + uses: actions/checkout@692973e3d937129bcbf40652eb9f2f61becf3332 # v4.1.7 + - name: Build and run unit tests + run: | + make build + make run-unit-tests + e2e-test: + runs-on: ubuntu-latest + steps: + - uses: actions/setup-go@0a12ed9d6a96ab950c8f026ed9f722fe0da7ef32 # v5.0.2 + with: + go-version: ${{ env.GO_VERSION }} + - name: Checkout finch-daemon repo + uses: actions/checkout@692973e3d937129bcbf40652eb9f2f61becf3332 # v4.1.7 + - name: Install Dependencies for e2e Testing + run: ./setup-test-env.sh + - name: Build the daemon + run: make build + - name: Remove default podman network config + run: | + sudo ls /etc/cni/net.d + sudo rm /etc/cni/net.d/87-podman-bridge.conflist + - name: Start finch-daemon + run: sudo bin/finch-daemon --debug --socket-owner $UID & + - name: Run e2e test + run: sudo make run-e2e-tests diff --git a/.golangci.yaml b/.golangci.yaml index 937678a..6e4f547 100644 --- a/.golangci.yaml +++ b/.golangci.yaml @@ -1,4 +1,5 @@ -# The sections in this file are ordered in the order presented in https://golangci-lint.run/usage/configuration/. +# The sections in this file are ordered in the order presented in +# https://golangci-lint.run/usage/configuration/. # The nested fields are ordered alphabetically. linters-settings: @@ -13,6 +14,7 @@ linters-settings: - G304 - G101 - G104 + - G115 # added to prevent false uint conversion errors lll: line-length: 250 tab-width: 4 @@ -23,12 +25,11 @@ linters-settings: require-specific: true stylecheck: checks: ["all", "-ST1003", "-ST1000", "-ST1001", "-ST1021"] - linters: enable: # - errname # - errorlint - - exportloopref + - copyloopvar # - forcetypeassert # - gocritic - godot @@ -56,16 +57,17 @@ linters: - stylecheck disable: - errcheck - issues: exclude-rules: - linters: - lll - # A go:generate statement has to be in the same line: https://github.com/golang/go/issues/46050. + # A go:generate statement has to be in the same line: + # https://github.com/golang/go/issues/46050. source: "^//go:generate " - # Some checks enabled in the stylecheck setting are disabled by default # (e.g., https://golangci-lint.run/usage/false-positives/#exc0013), # so we need to enable them explicitly here. exclude-use-default: false # fix: true +run: + timeout: 5m diff --git a/api/handlers/container/attach.go b/api/handlers/container/attach.go index 62f082e..c407cd5 100644 --- a/api/handlers/container/attach.go +++ b/api/handlers/container/attach.go @@ -104,10 +104,7 @@ func checkUpgradeStatus(ctx context.Context, upgrade bool) (string, string) { if versions.GreaterThanOrEqualTo(httputils.VersionFromContext(ctx), "1.42") { contentType = "application/vnd.docker.multiplexed-stream" } - successResponse = fmt.Sprintf("HTTP/1.1 101 UPGRADED\r\n" + - "Content-Type: " + contentType + "\r\n" + - "Connection: Upgrade\r\n" + - "Upgrade: tcp\r\n\r\n") + successResponse = fmt.Sprintf("HTTP/1.1 101 UPGRADED\r\nContent-Type: %s\r\nConnection: Upgrade\r\nUpgrade: tcp\r\n\r\n", contentType) } return contentType, successResponse } diff --git a/api/handlers/container/attach_test.go b/api/handlers/container/attach_test.go index dddb296..1ccf05d 100644 --- a/api/handlers/container/attach_test.go +++ b/api/handlers/container/attach_test.go @@ -72,7 +72,7 @@ var _ = Describe("Container Attach API", func() { expErrCode := http.StatusInternalServerError expErrMsg := "error" service.EXPECT().Attach(gomock.Any(), gomock.Any(), gomock.Any()). - Return(fmt.Errorf(expErrMsg)) + Return(fmt.Errorf("%s", expErrMsg)) req, _ = http.NewRequest(http.MethodPost, "/containers/123", nil) h.attach(rr, req) @@ -86,7 +86,7 @@ var _ = Describe("Container Attach API", func() { expErrCode := http.StatusNotFound expErrMsg := fmt.Sprintf("no container is found given the string: %s", "123") service.EXPECT().Attach(gomock.Any(), gomock.Any(), gomock.Any()). - Return(errdefs.NewNotFound(fmt.Errorf(expErrMsg))) + Return(errdefs.NewNotFound(fmt.Errorf("%s", expErrMsg))) req, _ = http.NewRequest(http.MethodPost, "/containers/123", nil) h.attach(rr, req) @@ -243,7 +243,7 @@ func (h *errorResponseRecorder) Code() int { } func (h *errorResponseRecorder) Hijack() (net.Conn, *bufio.ReadWriter, error) { - return nil, nil, fmt.Errorf(errRRErrMsg) + return nil, nil, fmt.Errorf("%s", errRRErrMsg) } // attachOptsMatcher is adapted from container create to be a wrapper type to diff --git a/api/handlers/container/create.go b/api/handlers/container/create.go index 4497f0d..5b56fa2 100644 --- a/api/handlers/container/create.go +++ b/api/handlers/container/create.go @@ -157,7 +157,8 @@ func (h *handler) create(w http.ResponseWriter, r *http.Request) { Env: env, Workdir: req.WorkingDir, Entrypoint: req.Entrypoint, - EntrypointChanged: req.Entrypoint != nil && len(req.Entrypoint) > 0, + EntrypointChanged: len(req.Entrypoint) > 0, + //req.Entrypoint != nil is removed because len(x) > 0 will automatically verify that req.Entrypoint is not nil // #endregion // #region for metadata flags diff --git a/api/handlers/container/list_test.go b/api/handlers/container/list_test.go index de29000..7826e43 100644 --- a/api/handlers/container/list_test.go +++ b/api/handlers/container/list_test.go @@ -137,7 +137,7 @@ var _ = Describe("Container List API", func() { Filters: nil, } errorMsg := "error from ListContainers" - service.EXPECT().List(gomock.Any(), listOpts).Return(nil, fmt.Errorf(errorMsg)) + service.EXPECT().List(gomock.Any(), listOpts).Return(nil, fmt.Errorf("%s", errorMsg)) h.list(rr, req) Expect(rr.Body).Should(MatchJSON(`{"message": "` + errorMsg + `"}`)) diff --git a/api/handlers/container/logs_test.go b/api/handlers/container/logs_test.go index 5471f1c..ec7ea3b 100644 --- a/api/handlers/container/logs_test.go +++ b/api/handlers/container/logs_test.go @@ -70,7 +70,7 @@ var _ = Describe("Container Logs API", func() { expErrCode := http.StatusInternalServerError expErrMsg := "error" service.EXPECT().Logs(gomock.Any(), gomock.Any(), gomock.Any()). - Return(fmt.Errorf(expErrMsg)) + Return(fmt.Errorf("%s", expErrMsg)) h.logs(rr, req) @@ -83,7 +83,7 @@ var _ = Describe("Container Logs API", func() { expErrCode := http.StatusNotFound expErrMsg := fmt.Sprintf("no container is found given the string: %s", "123") service.EXPECT().Logs(gomock.Any(), gomock.Any(), gomock.Any()). - Return(errdefs.NewNotFound(fmt.Errorf(expErrMsg))) + Return(errdefs.NewNotFound(fmt.Errorf("%s", expErrMsg))) h.logs(rr, req) diff --git a/api/handlers/exec/start_test.go b/api/handlers/exec/start_test.go index a49f38b..a2ed482 100644 --- a/api/handlers/exec/start_test.go +++ b/api/handlers/exec/start_test.go @@ -140,7 +140,7 @@ var _ = Describe("Exec Start API", func() { It("should return 500 if Hijacking the connection fails", func() { hijackErrMsg := "error hijacking the connection" errRR := newResponseRecorderWithMockHijack(rr, func() (net.Conn, *bufio.ReadWriter, error) { - return nil, nil, fmt.Errorf(hijackErrMsg) + return nil, nil, fmt.Errorf("%s", hijackErrMsg) }) h.start(errRR, req) diff --git a/api/handlers/image/push_test.go b/api/handlers/image/push_test.go index 3fa90bd..06b5bdd 100644 --- a/api/handlers/image/push_test.go +++ b/api/handlers/image/push_test.go @@ -164,7 +164,7 @@ var _ = Describe("Image Push API", func() { Expect(authCfg.Password).Should(BeEmpty()) // mimic service is trying to push the image and failed with auth error. outStream.Write([]byte(streamMsg)) - return nil, fmt.Errorf(errMsg) + return nil, fmt.Errorf("%s", errMsg) }) // handler should return error message with 500 status code diff --git a/api/handlers/network/network_test.go b/api/handlers/network/network_test.go index ba51c27..8bdbe4f 100644 --- a/api/handlers/network/network_test.go +++ b/api/handlers/network/network_test.go @@ -80,7 +80,7 @@ var _ = Describe("Network API ", func() { It("should call the network list handler using /networks", func() { // setup mocks expErr := "error from List" - service.EXPECT().List(gomock.Any()).Return(nil, fmt.Errorf(expErr)) + service.EXPECT().List(gomock.Any()).Return(nil, fmt.Errorf("%s", expErr)) req, _ = http.NewRequest(http.MethodGet, "/networks", nil) // call api and check if it returns error router.ServeHTTP(rr, req) @@ -90,7 +90,7 @@ var _ = Describe("Network API ", func() { It("should call the network list handler using /networks/", func() { // setup mocks expErr := "error from List" - service.EXPECT().List(gomock.Any()).Return(nil, fmt.Errorf(expErr)) + service.EXPECT().List(gomock.Any()).Return(nil, fmt.Errorf("%s", expErr)) req, _ = http.NewRequest(http.MethodGet, "/networks/", nil) // call api and check if it returns error router.ServeHTTP(rr, req) diff --git a/e2e/tests/tests.go b/e2e/tests/tests.go index 74f0b1c..0a2e28d 100644 --- a/e2e/tests/tests.go +++ b/e2e/tests/tests.go @@ -174,13 +174,13 @@ func fileShouldExistInContainer(opt *option.Option, containerName, path, content gomega.Expect(command.StdoutStr(opt, "exec", containerName, "cat", path)).To(gomega.Equal(content)) } -//nolint: unused // reserved for future use cases +//nolint:unused // reserved for future use cases func fileShouldNotExistInContainer(opt *option.Option, containerName, path string) { cmdOut := command.RunWithoutSuccessfulExit(opt, "exec", containerName, "cat", path) gomega.Expect(cmdOut.Err.Contents()).To(gomega.ContainSubstring("No such file or directory")) } -//nolint: unused // reserved for future use cases +//nolint:unused // reserved for future use cases func buildImage(opt *option.Option, imageName string) { dockerfile := fmt.Sprintf(`FROM %s CMD ["echo", "finch-test-dummy-output"] diff --git a/go.mod b/go.mod index 80e2f9a..aa325ce 100644 --- a/go.mod +++ b/go.mod @@ -1,6 +1,6 @@ module github.com/runfinch/finch-daemon -go 1.21.0 +go 1.22.7 require ( github.com/containerd/cgroups/v3 v3.0.3 @@ -147,4 +147,4 @@ require ( gopkg.in/yaml.v3 v3.0.1 // indirect k8s.io/cri-api v0.29.3 // indirect lukechampine.com/blake3 v1.2.1 // indirect -) +) \ No newline at end of file diff --git a/internal/service/container/attach_test.go b/internal/service/container/attach_test.go index 19fc254..454c2ed 100644 --- a/internal/service/container/attach_test.go +++ b/internal/service/container/attach_test.go @@ -116,7 +116,7 @@ var _ = Describe("Container Attach API ", func() { cdClient.EXPECT().SearchContainer(gomock.Any(), gomock.Any()).Return([]containerd.Container{con}, nil) logger.EXPECT().Debugf(gomock.Any(), gomock.Any()).Return() con.EXPECT().ID().Return(cid) - ncClient.EXPECT().GetDataStore().Return("", fmt.Errorf(expErr)) + ncClient.EXPECT().GetDataStore().Return("", fmt.Errorf("%s", expErr)) logger.EXPECT().Debugf(gomock.Any(), gomock.Any()).Return() // set up options opts := attachTypes.AttachOptions{ @@ -244,7 +244,7 @@ var _ = Describe("Container Attach API ", func() { k8slabels.ContainerMetadataExtension: testAny, }, nil) cdClient.EXPECT().GetContainerStatus(gomock.Any(), gomock.Any()).Return(containerd.Running) - cdClient.EXPECT().GetContainerTaskWait(gomock.Any(), gomock.Any(), gomock.Any()).Return(nil, nil, fmt.Errorf(expErr)) + cdClient.EXPECT().GetContainerTaskWait(gomock.Any(), gomock.Any(), gomock.Any()).Return(nil, nil, fmt.Errorf("%s", expErr)) logger.EXPECT().Debugf(gomock.Any(), gomock.Any()).Return() // set up options diff --git a/internal/service/container/logs_test.go b/internal/service/container/logs_test.go index 321901a..6311488 100644 --- a/internal/service/container/logs_test.go +++ b/internal/service/container/logs_test.go @@ -97,7 +97,7 @@ var _ = Describe("Container Logs API ", func() { cdClient.EXPECT().SearchContainer(gomock.Any(), gomock.Any()).Return([]containerd.Container{con}, nil) logger.EXPECT().Infof("getting logs for container: %s", cid).Return() con.EXPECT().ID().Return(cid) - ncClient.EXPECT().GetDataStore().Return("", fmt.Errorf(expErr)) + ncClient.EXPECT().GetDataStore().Return("", fmt.Errorf("%s", expErr)) logger.EXPECT().Debugf(gomock.Any(), gomock.Any()).Return() // set up options opts := types.LogsOptions{ @@ -233,7 +233,7 @@ var _ = Describe("Container Logs API ", func() { k8slabels.ContainerMetadataExtension: testAny, }, nil) cdClient.EXPECT().GetContainerStatus(gomock.Any(), gomock.Any()).Return(containerd.Running) - cdClient.EXPECT().GetContainerTaskWait(gomock.Any(), gomock.Any(), gomock.Any()).Return(nil, nil, fmt.Errorf(expErr)) + cdClient.EXPECT().GetContainerTaskWait(gomock.Any(), gomock.Any(), gomock.Any()).Return(nil, nil, fmt.Errorf("%s", expErr)) logger.EXPECT().Debugf(gomock.Any(), gomock.Any()).Return() // set up options diff --git a/internal/service/image/load_test.go b/internal/service/image/load_test.go index c48064e..f6fc394 100644 --- a/internal/service/image/load_test.go +++ b/internal/service/image/load_test.go @@ -18,7 +18,7 @@ import ( "github.com/runfinch/finch-daemon/mocks/mocks_logger" ) -// Unit tests related to image load API +// Unit tests related to image load API. var _ = Describe("Image Load API", func() { var ( ctx context.Context diff --git a/internal/service/network/list_test.go b/internal/service/network/list_test.go index 9d827ca..15b2e95 100644 --- a/internal/service/network/list_test.go +++ b/internal/service/network/list_test.go @@ -46,7 +46,7 @@ var _ = Describe("Network List API ", func() { }) It("should pass through errors from FilterNetworks", func() { expErr := "filter network error" - ncNetClient.EXPECT().FilterNetworks(gomock.Any()).Return(nil, fmt.Errorf(expErr)) + ncNetClient.EXPECT().FilterNetworks(gomock.Any()).Return(nil, fmt.Errorf("%s", expErr)) _, err := service.List(ctx) Expect(err.Error()).Should(Equal(expErr)) diff --git a/setup-test-env.sh b/setup-test-env.sh index 8339845..99d0bb3 100755 --- a/setup-test-env.sh +++ b/setup-test-env.sh @@ -40,4 +40,4 @@ export PATH=$PATH:/usr/local/bin sudo containerd & sudo buildkitd & -sleep 2 +sleep 2 \ No newline at end of file From d8b1994325476604bb2da125cb43e023a5e140e9 Mon Sep 17 00:00:00 2001 From: Arjun Raja Yogidas Date: Thu, 5 Sep 2024 20:50:26 +0000 Subject: [PATCH 2/2] add yamllint config Signed-off-by: Arjun Raja Yogidas --- .yamllint.yml | 10 ++++++++++ api/handlers/container/create.go | 1 - setup-test-env.sh | 2 +- 3 files changed, 11 insertions(+), 2 deletions(-) create mode 100644 .yamllint.yml diff --git a/.yamllint.yml b/.yamllint.yml new file mode 100644 index 0000000..c2dfe13 --- /dev/null +++ b/.yamllint.yml @@ -0,0 +1,10 @@ +extends: default +rules: + trailing-spaces: + # issue templates intentionally have some whitespace + ignore: .github/ISSUE_TEMPLATE/ + document-start: disable # preference, can enable if desired + comments: disable # doesn't like in-line comments like this + line-length: disable # hard to enforce <80 chars for some cmds + truthy: + allowed-values: ['true', 'false', 'on'] diff --git a/api/handlers/container/create.go b/api/handlers/container/create.go index 5b56fa2..de87e7e 100644 --- a/api/handlers/container/create.go +++ b/api/handlers/container/create.go @@ -158,7 +158,6 @@ func (h *handler) create(w http.ResponseWriter, r *http.Request) { Workdir: req.WorkingDir, Entrypoint: req.Entrypoint, EntrypointChanged: len(req.Entrypoint) > 0, - //req.Entrypoint != nil is removed because len(x) > 0 will automatically verify that req.Entrypoint is not nil // #endregion // #region for metadata flags diff --git a/setup-test-env.sh b/setup-test-env.sh index 99d0bb3..8339845 100755 --- a/setup-test-env.sh +++ b/setup-test-env.sh @@ -40,4 +40,4 @@ export PATH=$PATH:/usr/local/bin sudo containerd & sudo buildkitd & -sleep 2 \ No newline at end of file +sleep 2