-
Notifications
You must be signed in to change notification settings - Fork 3
Update CI and linter, fix issues #23
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -8,28 +8,18 @@ on: | |
| - created | ||
| env: | ||
| GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} | ||
| GO_VERSION: 1.20.x | ||
| GO_VERSION: stable | ||
| jobs: | ||
| build: | ||
| name: Upload Release Assets | ||
| runs-on: ubuntu-latest | ||
| steps: | ||
| - name: Install Go stable | ||
| if: env.GO_VERSION != 'tip' | ||
| uses: actions/setup-go@v4 | ||
| - name: Install Go | ||
| uses: actions/setup-go@v5 | ||
| with: | ||
| go-version: ${{ env.GO_VERSION }} | ||
| - name: Install Go tip | ||
| if: env.GO_VERSION == 'tip' | ||
| run: | | ||
| curl -sL https://storage.googleapis.com/go-build-snap/go/linux-amd64/$(git ls-remote https://github.com/golang/go.git HEAD | awk '{print $1;}').tar.gz -o gotip.tar.gz | ||
| ls -lah gotip.tar.gz | ||
| mkdir -p ~/sdk/gotip | ||
| tar -C ~/sdk/gotip -xzf gotip.tar.gz | ||
| ~/sdk/gotip/bin/go version | ||
| echo "PATH=$HOME/go/bin:$HOME/sdk/gotip/bin/:$PATH" >> $GITHUB_ENV | ||
| - name: Checkout code | ||
| uses: actions/checkout@v3 | ||
| uses: actions/checkout@v4 | ||
| - name: Build artifacts | ||
| run: | | ||
| make release-assets | ||
|
|
@@ -41,6 +31,14 @@ jobs: | |
| asset_path: ./linux_amd64.tar.gz | ||
| asset_name: linux_amd64.tar.gz | ||
| asset_content_type: application/tar+gzip | ||
| - name: Upload linux_amd64_dbg.tar.gz | ||
| if: hashFiles('linux_amd64_dbg.tar.gz') != '' | ||
| uses: actions/upload-release-asset@v1 | ||
| with: | ||
| upload_url: ${{ github.event.release.upload_url }} | ||
| asset_path: ./linux_amd64_dbg.tar.gz | ||
| asset_name: linux_amd64_dbg.tar.gz | ||
| asset_content_type: application/tar+gzip | ||
|
Comment on lines
+34
to
+41
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P2 | Confidence: Medium Speculative: The addition of debug binary uploads suggests the build process now generates debug artifacts, but there's no corresponding change in the Makefile to show how these are built. Without seeing the build configuration changes, it's unclear if debug symbols are properly stripped from release binaries or if debug builds follow security best practices (e.g., not including sensitive information). |
||
| - name: Upload linux_arm64.tar.gz | ||
| if: hashFiles('linux_arm64.tar.gz') != '' | ||
| uses: actions/upload-release-asset@v1 | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -15,36 +15,25 @@ concurrency: | |
| env: | ||
| GO111MODULE: "on" | ||
| RUN_BASE_COVERAGE: "on" # Runs test for PR base in case base test coverage is missing. | ||
| COV_GO_VERSION: 1.20.x # Version of Go to collect coverage | ||
| COV_GO_VERSION: stable # Version of Go to collect coverage | ||
| TARGET_DELTA_COV: 90 # Target coverage of changed lines, in percents | ||
| jobs: | ||
| test: | ||
| strategy: | ||
| matrix: | ||
| go-version: [ 1.13.x, 1.19.x, 1.20.x ] | ||
| go-version: [ 1.13.x, stable, oldstable ] | ||
| runs-on: ubuntu-latest | ||
| steps: | ||
| - name: Install Go stable | ||
| if: matrix.go-version != 'tip' | ||
| uses: actions/setup-go@v4 | ||
| - name: Install Go | ||
| uses: actions/setup-go@v5 | ||
| with: | ||
| go-version: ${{ matrix.go-version }} | ||
|
|
||
| - name: Install Go tip | ||
| if: matrix.go-version == 'tip' | ||
| run: | | ||
| curl -sL https://storage.googleapis.com/go-build-snap/go/linux-amd64/$(git ls-remote https://github.com/golang/go.git HEAD | awk '{print $1;}').tar.gz -o gotip.tar.gz | ||
| ls -lah gotip.tar.gz | ||
| mkdir -p ~/sdk/gotip | ||
| tar -C ~/sdk/gotip -xzf gotip.tar.gz | ||
| ~/sdk/gotip/bin/go version | ||
| echo "PATH=$HOME/go/bin:$HOME/sdk/gotip/bin/:$PATH" >> $GITHUB_ENV | ||
|
|
||
| - name: Checkout code | ||
| uses: actions/checkout@v3 | ||
| uses: actions/checkout@v4 | ||
|
|
||
| - name: Go cache | ||
| uses: actions/cache@v3 | ||
| uses: actions/cache@v4 | ||
| with: | ||
| # In order: | ||
| # * Module download cache | ||
|
|
@@ -59,7 +48,7 @@ jobs: | |
| - name: Restore base test coverage | ||
| id: base-coverage | ||
| if: matrix.go-version == env.COV_GO_VERSION && github.event.pull_request.base.sha != '' | ||
| uses: actions/cache@v2 | ||
| uses: actions/cache@v4 | ||
| with: | ||
| path: | | ||
| unit-base.txt | ||
|
|
@@ -88,14 +77,15 @@ jobs: | |
| id: annotate | ||
| if: matrix.go-version == env.COV_GO_VERSION && github.event.pull_request.base.sha != '' | ||
| run: | | ||
| curl -sLO https://github.com/vearutop/gocovdiff/releases/download/v1.3.6/linux_amd64.tar.gz && tar xf linux_amd64.tar.gz | ||
| curl -sLO https://github.com/vearutop/gocovdiff/releases/download/v1.4.2/linux_amd64.tar.gz && tar xf linux_amd64.tar.gz && rm linux_amd64.tar.gz | ||
| gocovdiff_hash=$(git hash-object ./gocovdiff) | ||
| [ "$gocovdiff_hash" == "8e507e0d671d4d6dfb3612309b72b163492f28eb" ] || (echo "::error::unexpected hash for gocovdiff, possible tampering: $gocovdiff_hash" && exit 1) | ||
| git fetch origin master ${{ github.event.pull_request.base.sha }} | ||
| REP=$(./gocovdiff -cov unit.coverprofile -gha-annotations gha-unit.txt -delta-cov-file delta-cov-unit.txt -target-delta-cov ${TARGET_DELTA_COV}) | ||
| [ "$gocovdiff_hash" == "c37862c73a677e5a9c069470287823ab5bbf0244" ] || (echo "::error::unexpected hash for gocovdiff, possible tampering: $gocovdiff_hash" && exit 1) | ||
| # Fetch PR diff from GitHub API. | ||
| curl -s -H "Authorization: token ${{ secrets.GITHUB_TOKEN }}" -H "Accept: application/vnd.github.v3.diff" https://api.github.com/repos/${{ github.repository }}/pulls/${{ github.event.pull_request.number }} > pull_request.diff | ||
| REP=$(./gocovdiff -diff pull_request.diff -mod github.com/$GITHUB_REPOSITORY -cov unit.coverprofile -gha-annotations gha-unit.txt -delta-cov-file delta-cov-unit.txt -target-delta-cov ${TARGET_DELTA_COV}) | ||
|
Comment on lines
+80
to
+85
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P1 | Confidence: High The test coverage analysis now fetches PR diff via GitHub API instead of using git fetch, which changes the fundamental approach to determining code changes. This introduces a dependency on GitHub API rate limits and authentication that wasn't present before. The new method may fail if the GitHub token lacks permissions or if API limits are exceeded, potentially breaking the coverage analysis workflow. The removal of git fetch also means the analysis no longer has access to the actual base commit files, which could lead to inaccurate coverage calculations. |
||
| echo "${REP}" | ||
| cat gha-unit.txt | ||
| DIFF=$(test -e unit-base.txt && ./gocovdiff -func-cov unit.txt -func-base-cov unit-base.txt || echo "Missing base coverage file") | ||
| DIFF=$(test -e unit-base.txt && ./gocovdiff -mod github.com/$GITHUB_REPOSITORY -func-cov unit.txt -func-base-cov unit-base.txt || echo "Missing base coverage file") | ||
| TOTAL=$(cat delta-cov-unit.txt) | ||
| echo "rep<<EOF" >> $GITHUB_OUTPUT && echo "$REP" >> $GITHUB_OUTPUT && echo "EOF" >> $GITHUB_OUTPUT | ||
| echo "diff<<EOF" >> $GITHUB_OUTPUT && echo "$DIFF" >> $GITHUB_OUTPUT && echo "EOF" >> $GITHUB_OUTPUT | ||
|
|
@@ -130,7 +120,7 @@ jobs: | |
|
|
||
| - name: Upload code coverage | ||
| if: matrix.go-version == env.COV_GO_VERSION | ||
| uses: codecov/codecov-action@v1 | ||
| uses: codecov/codecov-action@v5 | ||
| with: | ||
| file: ./unit.coverprofile | ||
| files: ./unit.coverprofile | ||
| flags: unittests | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,73 +1,85 @@ | ||
| # See https://github.com/golangci/golangci-lint/blob/master/.golangci.example.yml | ||
| # See https://golangci-lint.run/docs/linters/configuration/ | ||
| version: "2" | ||
| run: | ||
| tests: true | ||
|
|
||
| linters-settings: | ||
| errcheck: | ||
| check-type-assertions: true | ||
| check-blank: true | ||
| gocyclo: | ||
| min-complexity: 20 | ||
| dupl: | ||
| threshold: 100 | ||
| misspell: | ||
| locale: US | ||
| unused: | ||
| check-exported: false | ||
| unparam: | ||
| check-exported: true | ||
| cyclop: | ||
| max-complexity: 15 | ||
|
|
||
| linters: | ||
| enable-all: true | ||
| default: all | ||
| disable: | ||
| - nilnil | ||
| - tparallel | ||
| - goerr113 | ||
| - err113 | ||
|
Comment on lines
5
to
+10
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P2 | Confidence: Medium The linter configuration switches from enabling specific linters to enabling all and then disabling many. This approach may catch more issues but could introduce noise from linters not suited to this codebase. The extensive disable list (28+ linters) suggests the 'enable all' approach might not be optimal for this project's coding style and could make the configuration harder to maintain. |
||
| - errorlint | ||
| - lll | ||
| - maligned | ||
| - gochecknoglobals | ||
| - gomnd | ||
| - wrapcheck | ||
| - paralleltest | ||
| - noinlineerr | ||
| - wsl_v5 | ||
| - funcorder | ||
| - copyloopvar | ||
| - depguard | ||
| - dupword | ||
| - errname | ||
| - exhaustruct | ||
| - forbidigo | ||
| - exhaustivestruct | ||
| - interfacer # deprecated | ||
| - forcetypeassert | ||
| - scopelint # deprecated | ||
| - ifshort # too many false positives | ||
| - golint # deprecated | ||
| - varnamelen | ||
| - tagliatelle | ||
| - errname | ||
| - gochecknoglobals | ||
| - intrange | ||
| - ireturn | ||
| - exhaustruct | ||
| - lll | ||
| - mnd | ||
| - nonamedreturns | ||
| - nosnakecase | ||
| - structcheck | ||
| - varcheck | ||
| - deadcode | ||
| - paralleltest | ||
| - recvcheck | ||
| - tagalign | ||
| - tagliatelle | ||
| - testableexamples | ||
| - dupword | ||
|
|
||
| issues: | ||
| exclude-use-default: false | ||
| exclude-rules: | ||
| - linters: | ||
| - gomnd | ||
| - goconst | ||
| - goerr113 | ||
| - noctx | ||
| - funlen | ||
| - dupl | ||
| - structcheck | ||
| - unused | ||
| - unparam | ||
| - nosnakecase | ||
| path: "_test.go" | ||
| - linters: | ||
| - errcheck # Error checking omitted for brevity. | ||
| - gosec | ||
| path: "example_" | ||
|
|
||
| - testifylint | ||
| - varnamelen | ||
| - wrapcheck | ||
| settings: | ||
| dupl: | ||
| threshold: 100 | ||
| errcheck: | ||
| check-type-assertions: true | ||
| check-blank: true | ||
| gocyclo: | ||
| min-complexity: 20 | ||
| cyclop: | ||
| max-complexity: 15 | ||
| misspell: | ||
| locale: US | ||
| unparam: | ||
| check-exported: true | ||
| exclusions: | ||
| generated: lax | ||
| rules: | ||
| - linters: | ||
| - gosec | ||
| - dupl | ||
| - funlen | ||
| - goconst | ||
| - mnd | ||
| - noctx | ||
| - unparam | ||
| - unused | ||
| path: _test.go | ||
| - linters: | ||
| - errcheck | ||
| - gosec | ||
| path: example_ | ||
| - linters: | ||
| - revive | ||
| text: 'unused-parameter: parameter' | ||
| paths: | ||
| - third_party$ | ||
| - builtin$ | ||
| - examples$ | ||
| formatters: | ||
| enable: | ||
| - gci | ||
| - gofmt | ||
| - gofumpt | ||
| - goimports | ||
| exclusions: | ||
| generated: lax | ||
| paths: | ||
| - third_party$ | ||
| - builtin$ | ||
| - examples$ | ||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -40,7 +40,7 @@ | |||||
|
|
||||||
| input = flag.Arg(0) | ||||||
| if input == "" { | ||||||
| _, _ = fmt.Fprintln(flag.CommandLine.Output(), "Missing input path argument, use `-` for stdin.") | ||||||
| _, _ = fmt.Fprintln(flag.CommandLine.Output(), "Missing input path argument, use `-` for stdin.") //nolint | ||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P2 | Confidence: High The blanket nolint directive suppresses all linter warnings without specifying which linter or warning is being ignored. This makes the code harder to maintain and could hide legitimate issues. Specific linters should be named in the nolint directive to ensure only intended warnings are suppressed.
Suggested change
Evidence: method:fmt.Fprintln, search:nolint |
||||||
| flag.Usage() | ||||||
|
|
||||||
| return | ||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -198,7 +198,7 @@ func (c Comparer) filterExpected(expected []byte) ([]byte, error) { | |||||
| return nil, fmt.Errorf("failed to marshal var %s: %v", k, err) // Not wrapping to support go1.12. | ||||||
| } | ||||||
|
|
||||||
| expected = bytes.Replace(expected, []byte(`"`+k+`"`), j, -1) //nolint:gocritic // To support go1.11. | ||||||
| expected = bytes.Replace(expected, []byte(`"`+k+`"`), j, -1) //nolint:gocritic,staticcheck // To support go1.11. | ||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P2 | Confidence: High The added staticcheck nolint directive suppresses a legitimate lint warning about using deprecated functionality (bytes.Replace with -1 count). Since the project now uses Go 1.17+ (per go.mod), this should be updated to use bytes.ReplaceAll() for better clarity and forward compatibility, as the current approach relies on deprecated behavior.
Suggested change
Evidence: method:bytes.Replace, search:gocritic,staticcheck |
||||||
| } | ||||||
| } | ||||||
|
|
||||||
|
|
||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
P1 | Confidence: High
The change replaces specific Go version pinning (1.20.x) with 'stable', which introduces non-deterministic build behavior. This could cause the gorelease tool to analyze the code against different Go versions over time, potentially leading to inconsistent API compatibility reports. For a release tooling workflow, deterministic behavior is critical to ensure consistent analysis results across different runs.
Evidence: constant:GO_VERSION, search:stable