Skip to content
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

Jump to bottom

smoke test:fix license header check #13956

Merged
merged 1 commit into from
Apr 19, 2022
Merged

smoke test:fix license header check #13956

merged 1 commit into from
Apr 19, 2022

Conversation

vimalk78
Copy link
Contributor

partially fixes: #13896

This PR fixes make test-smoke pass for license_header

Fixed function go_srcs_in_module to return go files in the package.

@vimalk78 vimalk78 mentioned this pull request Apr 18, 2022
Open
@vimalk78
Copy link
Contributor Author

/retest

@codecov-commenter
Copy link

codecov-commenter commented Apr 18, 2022 •
edited
Loading

Codecov Report

Merging #13956 (a5f28b1) into main (8149191) will decrease coverage by 0.09%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main   #13956      +/-   ##
==========================================
- Coverage   72.61%   72.52%   -0.10%     
==========================================
  Files         469      469              
  Lines       38414    38414              
==========================================
- Hits        27895    27858      -37     
- Misses       8745     8777      +32     
- Partials     1774     1779       +5
Flag Coverage Δ
all 72.52% <ø> (-0.10%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
client/pkg/v3/tlsutil/tlsutil.go 83.33% <0.00%> (-8.34%) ⬇️
client/v3/namespace/watch.go 87.87% <0.00%> (-6.07%) ⬇️
client/v3/leasing/cache.go 87.77% <0.00%> (-3.89%) ⬇️
client/pkg/v3/testutil/recorder.go 76.27% <0.00%> (-3.39%) ⬇️
server/etcdserver/cluster_util.go 70.35% <0.00%> (-3.17%) ⬇️
server/etcdserver/api/v3election/election.go 66.66% <0.00%> (-2.78%) ⬇️
client/pkg/v3/transport/listener.go 57.18% <0.00%> (-2.40%) ⬇️
server/etcdserver/util.go 86.17% <0.00%> (-2.13%) ⬇️
pkg/traceutil/trace.go 96.15% <0.00%> (-1.93%) ⬇️
server/etcdserver/api/v3rpc/watch.go 86.57% <0.00%> (-1.35%) ⬇️
... and 11 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8149191...a5f28b1. Read the comment docs.

@ptabor
Copy link
Contributor

ptabor commented Apr 18, 2022

Thank you, Vimal.

What are the exact commands executed when the problem manifests ?

I tried to repro and it works for me before the patch:

'license_header' started at Mon Apr 18 10:03:04 CEST 2022
% (cd tools/mod && go install github.com/google/addlicense)
% (cd api && /Users/ptab/go/bin/addlicense --check v3rpc/rpctypes/doc.go v3rpc/rpctypes/error.go etcdserverpb/raft_internal_stringer.go v3rpc/rpctypes/metadatafields.go version/version.go v3rpc/rpctypes/md.go)
% (cd tools/mod && go install github.com/google/addlicense)
% (cd pkg && /Users/ptab/go/bin/addlicense --check adt/interval_tree.go cpuutil/doc.go cpuutil/endian.go crc/crc.go debugutil/doc.go debugutil/pprof.go expect/expect.go flags/flag.go cobrautl/error.go contention/doc.go adt/adt.go flags/strings.go flags/urls.go contention/contention.go flags/unique_strings.go grpc_testing/stub_server.go cobrautl/help.go idutil/id.go ioutil/pagewriter.go ioutil/readcloser.go flags/unique_urls.go ioutil/util.go ioutil/reader.go netutil/netutil.go flags/ignored.go netutil/routes.go netutil/routes_linux.go netutil/doc.go grpc_testing/recorder.go osutil/interrupt_windows.go osutil/osutil.go osutil/signal.go osutil/signal_linux.go proxy/doc.go osutil/interrupt_unix.go notify/notify.go report/report.go httputil/httputil.go proxy/server.go runtime/fds_linux.go runtime/fds_other.go schedule/doc.go pbutil/pbutil.go stringutil/rand.go flags/selective_string.go wait/wait_time.go stringutil/doc.go schedule/schedule.go report/doc.go wait/wait.go report/timeseries.go traceutil/trace.go report/weighted.go)

@vimalk78
Copy link
Contributor Author

vimalk78 commented Apr 18, 2022 •
edited
Loading

@ptabor my go env is

$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/vimalkum/.cache/go-build"
GOENV="/home/vimalkum/.config/go/env"
GOEXE=""
GOEXPERIMENT=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GOMODCACHE="/home/vimalkum/go/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/home/vimalkum/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/local/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/go/pkg/tool/linux_amd64"
GOVCS=""
GOVERSION="go1.18"
GCCGO="gccgo"
GOAMD64="v1"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/home/vimalkum/go/src/github.com/etcd-io/etcd/go.mod"
GOWORK=""
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build3197081312=/tmp/go-build -gno-record-gcc-switches"

issue i faced on the main is :

 $ PASSES="license_header" ./scripts/test.sh
Running with --race
Starting at: Mon Apr 18 08:01:27 PM IST 2022

'license_header' started at Mon Apr 18 08:01:27 PM IST 2022
% (cd tools/mod && 'go' 'install' 'github.com/google/addlicense')
% (cd api && '/home/vimalkum/go/bin/addlicense' '--check')
stderr: Usage: addlicense [flags] pattern [pattern ...]
stderr: 
stderr: The program ensures source code files have copyright license headers
stderr: by scanning directory patterns recursively.
stderr: 
stderr: It modifies all source files in place and avoids adding a license header
stderr: to any file that already has one.
stderr: 
stderr: The pattern argument can be provided multiple times, and may also refer
stderr: to single files.
stderr: 
stderr: Flags:
stderr: 
stderr: -c string
FAIL: (code:1):
  % (cd api && '/home/vimalkum/go/bin/addlicense' '--check')
stderr: copyright holder (default "Google LLC")
stderr: -check
stderr: check only mode: verify presence of license headers and exit with non-zero code if missing
stderr: -f string
stderr: license file
stderr: -ignore value
stderr: file patterns to ignore, for example: -ignore **/*.go -ignore vendor/**
stderr: -l string
stderr: license type: apache, bsd, mit, mpl (default "apache")
stderr: -s	Include SPDX identifier in license header. Set -s=only to only include SPDX identifier.
stderr: -skip value
stderr: [deprecated: see -ignore] file extensions to skip, for example: -skip rb -skip go
stderr: -v	verbose mode: print the name of the files that are modified
stderr: -y string
stderr: copyright year(s) (default "2022")

FAIL: 'license_header_per_module ./...' checking failed (!=0 return code)
FAIL: 'license_header' failed at Mon Apr 18 08:01:28 PM IST 2022

same is faced perhaps by @willbeason

The problem is that the function go_srcs_in_module , the expression go fmt -n "$1" | grep -Eo "([^ ]*)$" returns an empty string. So there are no files being passed to addlicense, hence it shows usage and exits with error.

Problem is seen in make test-smoke

@ptabor
Copy link
Contributor

ptabor commented Apr 18, 2022

It seems to work for me... but it's pretty subtle to parse output of go fmt.

Could you, please, check whether following construction is working for you ?

go list -f '{{with $c:=.}}{{range $f:=$c.GoFiles  }}{{$c.Dir}}/{{$f}}{{"\n"}}{{end}}{{end}}' ./... | grep -v ".pb.go"

@vimalk78
Copy link
Contributor Author

vimalk78 commented Apr 18, 2022 •
edited by ptabor
Loading

i tried your suggestion, it works. But does it need to filter out *.pb.gw.go also? as currently the script filters these files also

Yes - please filter pb.gw.go as well.

@vimalk78
Copy link
Contributor Author

i did. it works. shall i update the PR with go list -f as you suggested?

@vimalk78 vimalk78 force-pushed the fix-license-header-check branch from a5f28b1 to e5a9f3d Compare April 18, 2022 19:08
@vimalk78
Copy link
Contributor Author

i did. it works. shall i update the PR with go list -f as you suggested?

updated the PR. PTAL

ptabor
ptabor reviewed Apr 18, 2022
View reviewed changes
scripts/test_lib.sh
@@ -108,7 +108,7 @@ function relativePath {
# go_srcs_in_module [package]
# returns list of all not-generated go sources in the current (dir) module.
function go_srcs_in_module {
go fmt -n "$1" | grep -Eo "([^ ]*)$" | grep -vE "(\\_test.go|\\.pb\\.go|\\.pb\\.gw.go)"
Copy link
Contributor

Choose a reason for hiding this comment

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

Looking at: https://github-wiki-see.page/m/koalaman/shellcheck/wiki/SC2016

We need to change quotes to avoid warning:

go list -f "{{with \$c:=.}}{{range \$f:=\$c.GoFiles }}{{\$c.Dir}}/{{\$f}}{{\"\n\"}}{{end}}{{end}}" ./...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks @ptabor , updated PR

@vimalk78 vimalk78 force-pushed the fix-license-header-check branch from e5a9f3d to 12ab72a Compare April 19, 2022 03:05
@ptabor ptabor merged commit 1b28597 into etcd-io:main Apr 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ptabor ptabor ptabor left review comments

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Improve contributor experience with CI
3 participants
@vimalk78 @codecov-commenter @ptabor