diff --git a/.github/workflows/knative-style.yaml b/.github/workflows/knative-style.yaml index 31a0aeb2..be0bd9dd 100644 --- a/.github/workflows/knative-style.yaml +++ b/.github/workflows/knative-style.yaml @@ -26,9 +26,20 @@ jobs: autoformat: name: Auto-format and Check runs-on: ubuntu-latest + strategy: + fail-fast: false # Keep running if one leg fails. + matrix: + tool: + - goimports + - gofmt + + include: + - tool: gofmt + options: -s + - tool: goimports + importpath: golang.org/x/tools/cmd/goimports steps: - - name: Set up Go 1.15.x uses: actions/setup-go@v2 with: @@ -39,41 +50,30 @@ jobs: uses: actions/checkout@v2 - name: Install Dependencies + if: ${{ matrix.importpath != '' }} run: | cd $(mktemp -d) - GO111MODULE=on go get golang.org/x/tools/cmd/goimports + GO111MODULE=on go get ${{ matrix.importpath }} - # Run this last because it alters the workspace. - # TODO: add prettier step - - name: Go Imports + - name: ${{ matrix.tool }} ${{ matrix.options }} shell: bash run: | - goimports -w $(find -path './vendor' -prune -o -path './third_party' -prune -o -name '*.pb.go' -prune -o -type f -name '*.go' -print) - echo "::set-env name=FMT_TOOL::goimports" - - name: Verify goimport - shell: bash - run: | - if [[ $(git diff-index --name-only HEAD --) ]]; then - echo "Found diffs in:" - git diff-index --name-only HEAD -- - echo "${{ github.repository }} is out of style. Please run $FMT_TOOL." - exit 1 - fi - echo "${{ github.repository }} is formatted correctly." + ${{ matrix.tool }} ${{ matrix.options }} -w $(find . -path './vendor' -prune -o -path './third_party' -prune -o -name '*.pb.go' -prune -o -type f -name '*.go' -print) - - name: Go Format + - name: Verify ${{ matrix.tool }} shell: bash run: | - gofmt -s -w $(find -path './vendor' -prune -o -path './third_party' -prune -o -name '*.pb.go' -prune -o -type f -name '*.go' -print) - echo "::set-env name=FMT_TOOL::gofmt" - - name: Verify gofmt - shell: bash - # TODO(mattmoor): combine with above via anchors when actions support it. - run: | + # From: https://backreference.org/2009/12/23/how-to-match-newlines-in-sed/ + # This is to leverage this workaround: + # https://github.com/actions/toolkit/issues/193#issuecomment-605394935 + function urlencode() { + sed ':begin;$!N;s/\n/%0A/;tbegin' + } if [[ $(git diff-index --name-only HEAD --) ]]; then - echo "Found diffs in:" - git diff-index --name-only HEAD -- - echo "${{ github.repository }} is out of style. Please run $FMT_TOOL." + for x in $(git diff-index --name-only HEAD --); do + echo "::error file=$x::Please run ${{ matrix.tool }} ${{ matrix.options }}.%0A$(git diff $x | urlencode)" + done + echo "${{ github.repository }} is out of style. Please run ${{ matrix.tool }} ${{ matrix.options }}." exit 1 fi echo "${{ github.repository }} is formatted correctly." @@ -120,7 +120,6 @@ jobs: uses: golangci/golangci-lint-action@v2 with: version: v1.30 - only-new-issues: true # for initial defensiveness - name: misspell shell: bash @@ -197,6 +196,40 @@ jobs: echo '::endgroup::' + - name: Redundant Format + shell: bash + if: ${{ always() }} + env: + REVIEWDOG_GITHUB_API_TOKEN: ${{ github.token }} + run: | + set -e + cd "${GITHUB_WORKSPACE}" || exit 1 + + echo '::group:: Flagging possibly unnecessary format calls for trailing argument with reviewdog 🐶 ...' + # Don't fail because of misspell + set +o pipefail + # For all look for formatting calls and extract the ones that + # have only a single formatting argument and that argument is in the last position. + # But exclude the `fmt.Errorf` calls since there's no `fmt.Error` and `errors.New` is not + # as versatile. + # Also ignore `%T` — to print the type and `%q` to quote the final string. + fn=$(find . -path './vendor' -prune -o -path './third_party' -prune -o -name '*.pb.go' -prune -o -type f -name '*.go' -print) + grep -nE '[^.]+\.(Fatalf|Errorf|Warnf|Infof|Debugf|Logf|Sprintf|Fprintf|Printf)[^\%]+%[^Tqx]",[^,]+' $fn | grep -v "fmt.Errorf" | + while read -r ent ; do + file=$(echo $ent | cut -d':' -f 1); + line=$(echo $ent | cut -d':' -f 2); + ch=$(echo $ent | cut -d':' -f3-); + err=$(echo $ch | sed -E 's/([^.]+\.)(Fatal|Error|Warn|Info|Debug|Log|Sprint|Print|Fprint)f([^\%]+)(%[^wq]",)([^,]+)/\1\2\3",\5/'); + echo "$file:$line: Please consider avoiding tail format like this:%0A$err" + done | + reviewdog -efm="%f:%l: %m" \ + -name="Redundant Format" \ + -reporter="github-pr-check" \ + -filter-mode="added" \ + -fail-on-error="true" \ + -level="error" + echo '::endgroup::' + # This is mostly copied from https://github.com/get-woke/woke-action-reviewdog/blob/main/entrypoint.sh # since their action is not yet released under a stable version. - name: Language diff --git a/.github/workflows/knative-verify.yaml b/.github/workflows/knative-verify.yaml index c07f7462..7b9664ad 100644 --- a/.github/workflows/knative-verify.yaml +++ b/.github/workflows/knative-verify.yaml @@ -63,12 +63,23 @@ jobs: - name: Verify shell: bash run: | + # From: https://backreference.org/2009/12/23/how-to-match-newlines-in-sed/ + # This is to leverage this workaround: + # https://github.com/actions/toolkit/issues/193#issuecomment-605394935 + function urlencode() { + sed ':begin;$!N;s/\n/%0A/;tbegin' + } + pushd ./src/knative.dev/${{ github.event.repository.name }} if [[ -z "$(git status --porcelain)" ]]; then echo "${{ github.repository }} up to date." else - repoDiff=$(git diff-index --name-only HEAD --) - echo "Found diffs in: $repoDiff" + # Print it both ways because sometimes we haven't modified the file (e.g. zz_generated), + # and sometimes we have (e.g. configmap checksum). + echo "Found diffs in: $(git diff-index --name-only HEAD --)" + for x in $(git diff-index --name-only HEAD --); do + echo "::error file=$x::Please run ./hack/update-codegen.sh.%0A$(git diff $x | urlencode)" + done echo "${{ github.repository }} is out of date. Please run hack/update-codegen.sh" exit 1 fi