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.

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

Linter version verification #1861

Merged
merged 73 commits into from
Aug 19, 2022
Merged

Conversation

mindovermiles262
Copy link
Contributor

Signed-off-by: Andrew [email protected]

Description

Uses docker, if available, to run the golangci-lint er which is in sync with version of linting on Github

Issue reference

#1859

Checklist

Please make sure you've completed the relevant tasks for this PR, out of the following list:

  • Code compiles correctly
  • Created/updated tests
  • Extended the documentation

@mindovermiles262 mindovermiles262 requested review from a team as code owners July 7, 2022 15:51
@mindovermiles262
Copy link
Contributor Author

User [~/code/go/components-contrib] (ft/docker-lint)
$ which docker
/usr/bin/docker

User [~/code/go/components-contrib] (ft/docker-lint)
$ make lint
# Due to https://github.com/golangci/golangci-lint/issues/580, we need to add --fix for windows
docker run --rm -v /home/dipsea/code/go/components-contrib:/app -w /app golangci/golangci-lint:v1.45.2 golangci-lint run --timeout=20m
[...]

User [~/code/go/components-contrib] (ft/docker-lint)
$ sudo mv /usr/bin/docker /usr/bin/dokr

User [~/code/go/components-contrib] (ft/docker-lint)
$ make lint
# Due to https://github.com/golangci/golangci-lint/issues/580, we need to add --fix for windows
golangci-lint run --timeout=20m
[...]

Makefile Outdated Show resolved Hide resolved
Makefile Show resolved Hide resolved
@ItalyPaleAle ItalyPaleAle changed the title use docker for linting for github action compatability use docker for linting for github action compatibility Jul 7, 2022
@mindovermiles262
Copy link
Contributor Author

I do not have a windows device to test the where golangci-lint.exe command or the installation/lint/grepping. Can anyone test that for me?

Otherwise, here are the logs from my local testing. Started first without the linter installed, then used the "correct" version, then a different version from the Github CI

$ which golangci-lint

$ make lint
[*] golangci_lint not installed
[*] Installing now...
golangci/golangci-lint info checking GitHub for tag 'v1.45.2'
golangci/golangci-lint info found version: 1.45.2 for v1.45.2/linux/amd64
golangci/golangci-lint info installed /home/dipsea/code/go/bin/golangci-lint
# Due to https://github.com/golangci/golangci-lint/issues/580, we need to add --fix for windows
golangci-lint run --timeout=20m
[ ...SNIP... Lint running with v1.45.2 ]




$ golangci-lint --version
golangci-lint has version 1.45.2 built from 8bdc4d3f on 2022-03-24T11:51:26Z

$ make lint
# Due to https://github.com/golangci/golangci-lint/issues/580, we need to add --fix for windows
golangci-lint run --timeout=20m
[ ...SNIP... Lint running with v1.45.2 ]




$ golangci-lint --version
golangci-lint has version 1.46.2 built from a3336890 on 2022-05-17T11:31:29Z

$ make lint
[!] Your locally installed version of golangci-lint is different from the pipeline
[!] This will likely cause linting issues for you locally
[!] Yours: v1.46.2 Theirs: v1.45.2
# Due to https://github.com/golangci/golangci-lint/issues/580, we need to add --fix for windows
golangci-lint run --timeout=20m
[ ...SNIP... Lint with Errors from using v1.46.2 ]

Makefile Outdated Show resolved Hide resolved
@mindovermiles262
Copy link
Contributor Author

Haven't gotten a chance to look at this yet, but still on my ToDo list

@berndverst
Copy link
Member

For the time being I will convert this PR to a draft. Once the changes have been made feel free to convert it back to a regular PR and we can get this merged.

@berndverst berndverst marked this pull request as draft August 8, 2022 17:04
@codecov
Copy link

codecov bot commented Aug 8, 2022

Codecov Report

Merging #1861 (a21456d) into master (8b48210) will increase coverage by 0.02%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #1861      +/-   ##
==========================================
+ Coverage   37.63%   37.66%   +0.02%     
==========================================
  Files         192      192              
  Lines       23982    23982              
==========================================
+ Hits         9026     9032       +6     
+ Misses      14191    14183       -8     
- Partials      765      767       +2     
Impacted Files Coverage Δ
nameresolution/mdns/mdns.go 71.74% <0.00%> (-0.52%) ⬇️
state/in-memory/in_memory.go 46.00% <0.00%> (+3.42%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@mindovermiles262 mindovermiles262 marked this pull request as ready for review August 18, 2022 21:59
@mindovermiles262
Copy link
Contributor Author

I think this is ready for review:

Linter Not Installed

--> Throws Error

% make lint
[!] golangci_lint not installed
[!] You can install it from https://golangci-lint.run/usage/install/
[!]   or by running
[!]   curl -sSfL https://raw.githubusercontent.com/golangci/golangci-lint/master/install.sh | sh -s -- -b /bin
make: *** [verify-linter-installed] Error 1

Linter Version Mismatch

--> Throws Warning, continues with lint

% make lint
[!] Your locally installed version of golangci-lint is different from the pipeline
[!] This will likely cause linting issues for you locally
[!] Yours:  v1.46.2
[!] Theirs: v1.48.0
# Due to https://github.com/golangci/golangci-lint/issues/580, we need to add --fix for windows
golangci-lint run --timeout=20m

@mindovermiles262 mindovermiles262 changed the title use docker for linting for github action compatibility Linter version verification Aug 18, 2022
@yaron2
Copy link
Member

yaron2 commented Aug 18, 2022

cc @berndverst

@berndverst
Copy link
Member

I saw @yaron2. I got lots of email notifications for every changed made to this 😄

@berndverst
Copy link
Member

@mindovermiles262 please make sure to sign off on your commits -- see the failing DCO check.

ItalyPaleAle and others added 12 commits August 18, 2022 17:09
* WIP: Azure AD support in SignalR

Signed-off-by: ItalyPaleAle <[email protected]>

* Correct SignalR AAD details

Signed-off-by: ItalyPaleAle <[email protected]>

* Misc fixes

Signed-off-by: ItalyPaleAle <[email protected]>

* azauth package name

Signed-off-by: ItalyPaleAle <[email protected]>
Signed-off-by: Andrew Duss <[email protected]>
Signed-off-by: Andrew Duss <[email protected]>
Signed-off-by: Bernd Verst <[email protected]>
Signed-off-by: Andrew Duss <[email protected]>
Signed-off-by: ItalyPaleAle <[email protected]>
Signed-off-by: Andrew Duss <[email protected]>
Signed-off-by: ItalyPaleAle <[email protected]>
Signed-off-by: Andrew Duss <[email protected]>
Signed-off-by: ItalyPaleAle <[email protected]>
Signed-off-by: Andrew Duss <[email protected]>
Signed-off-by: cmendible <[email protected]>
Signed-off-by: Andrew Duss <[email protected]>
Signed-off-by: Bernd Verst <[email protected]>
Signed-off-by: Andrew Duss <[email protected]>
* working with hardcoded SHA-512

Signed-off-by: Andrew Duss <[email protected]>

* cleanup code

Signed-off-by: Andrew Duss <[email protected]>

* Do not hardcode specific testTopicName

Signed-off-by: ItalyPaleAle <[email protected]>
Signed-off-by: Andrew Duss <[email protected]>

* Ensure context propagation in MySQL binding (dapr#1829)

Spin-off from PR adding contexts to input bindings

Signed-off-by: ItalyPaleAle <[email protected]>

Co-authored-by: Dapr Bot <[email protected]>
Signed-off-by: Andrew Duss <[email protected]>

* Add support for AAD auth in Azure Storage Queues binding (dapr#1842)

* Add support for AAD auth in Azure Storage Queues binding

Signed-off-by: ItalyPaleAle <[email protected]>

* 🧹

Signed-off-by: ItalyPaleAle <[email protected]>

Co-authored-by: Bernd Verst <[email protected]>
Signed-off-by: Andrew Duss <[email protected]>

* Moved authentication to be an internal pkg (dapr#1855)

Signed-off-by: ItalyPaleAle <[email protected]>
Signed-off-by: Andrew Duss <[email protected]>

* Azure AD support in SignalR (dapr#1852)

* WIP: Azure AD support in SignalR

Signed-off-by: ItalyPaleAle <[email protected]>

* Correct SignalR AAD details

Signed-off-by: ItalyPaleAle <[email protected]>

* Misc fixes

Signed-off-by: ItalyPaleAle <[email protected]>

* azauth package name

Signed-off-by: ItalyPaleAle <[email protected]>
Signed-off-by: Andrew Duss <[email protected]>

* rename SCRAM properly as SASL

Signed-off-by: Andrew Duss <[email protected]>

* update gomod/sum

Signed-off-by: Andrew Duss <[email protected]>

* gofmt

Signed-off-by: Andrew Duss <[email protected]>

* mod tidy

Signed-off-by: Andrew Duss <[email protected]>

* goval

Signed-off-by: Andrew Duss <[email protected]>

* use xdg-go instead of depreicated xdg library

Signed-off-by: Andrew Duss <[email protected]>

Co-authored-by: ItalyPaleAle <[email protected]>
Co-authored-by: Dapr Bot <[email protected]>
Co-authored-by: Bernd Verst <[email protected]>
Co-authored-by: Yaron Schneider <[email protected]>
Signed-off-by: Andrew Duss <[email protected]>
1046102779 and others added 14 commits August 18, 2022 17:09
* optimize: skip fatal when pubsub/jetstream server don't start in unit test

Signed-off-by: 1046102779 <[email protected]>

* optimize: skip fatal when pubsub/jetstream server don't start in unit test

Signed-off-by: 1046102779 <[email protected]>

* optimize: skip fatal when pubsub/jetstream server don't start in unit test

Signed-off-by: 1046102779 <[email protected]>

* optimize: add e2etests buildtags for e2e

Signed-off-by: 1046102779 <[email protected]>

Signed-off-by: 1046102779 <[email protected]>
Co-authored-by: Loong Dai <[email protected]>
Co-authored-by: Bernd Verst <[email protected]>
Signed-off-by: Andrew Duss <[email protected]>
* Add service bus exported error handling

Signed-off-by: Addison Juarez <[email protected]>

* Add service bus exported error handling

Signed-off-by: Addison Juarez <[email protected]>

Signed-off-by: Addison Juarez <[email protected]>
Co-authored-by: Loong Dai <[email protected]>
Signed-off-by: Andrew Duss <[email protected]>
* pubsub.in-memory: add support for wildcard topics

Fixes dapr#1964

Signed-off-by: ItalyPaleAle <[email protected]>

* make modtidy-all

Signed-off-by: Bernd Verst <[email protected]>

Signed-off-by: ItalyPaleAle <[email protected]>
Signed-off-by: Bernd Verst <[email protected]>
Co-authored-by: Bernd Verst <[email protected]>
Signed-off-by: Andrew Duss <[email protected]>
PR dapr/dapr#4989 (issue dapr/dapr#4988) updated the names of some
enums in `UnlockResponse.Status` ProtoBuff definition:

* `LOCK_UNEXIST` became `LOCK_DOES_NOT_EXIST `
* `LOCK_BELONG_TO_OTHERS`  became `LOCK_BELONGS_TO_OTHERS`

Code implementing Distributed Lock component in components-contrib
needs to be updated to account for this modification.

This PR accomplishes this by updating contants mapping back to the
two enums above to match their new names.

Fixes components-contrib#1950

Signed-off-by: Tiago Alves Macambira <[email protected]>

Signed-off-by: Tiago Alves Macambira <[email protected]>
Co-authored-by: Loong Dai <[email protected]>
Co-authored-by: Bernd Verst <[email protected]>
Signed-off-by: Andrew Duss <[email protected]>
…iled (dapr#1965)

* fix(kafka): ConsumeClaim should not return err when retry recovery failed

Signed-off-by: zcong1993 <[email protected]>

* chore: tweak error log message

Signed-off-by: zcong1993 <[email protected]>

Signed-off-by: zcong1993 <[email protected]>
Co-authored-by: Bernd Verst <[email protected]>
Co-authored-by: Dapr Bot <[email protected]>
Signed-off-by: Andrew Duss <[email protected]>
* Allow metadata to flow through service bus queue

This commit lets metadata bind to a message and be read when
the message is received. This allows for the distributed tracing
of events.

Signed-off-by: Hal Spang <[email protected]>

* Remove saved fields from ApplicationProperties

Signed-off-by: Hal Spang <[email protected]>

* Add a certification test for metadata

Signed-off-by: Hal Spang <[email protected]>

Signed-off-by: Hal Spang <[email protected]>
Co-authored-by: Alessandro (Ale) Segala <[email protected]>
Co-authored-by: Bernd Verst <[email protected]>
Signed-off-by: Andrew Duss <[email protected]>
* Go 1.19 support and linter fixes

Signed-off-by: Bernd Verst <[email protected]>

* Update workflows for Go1.19 and new linter version

Signed-off-by: Bernd Verst <[email protected]>

* Remove unnecessary space in nolint directive

Signed-off-by: Bernd Verst <[email protected]>

* disable additional linters which aren't used because of Go generics

Signed-off-by: Bernd Verst <[email protected]>

* enable gosec linter again

Signed-off-by: Bernd Verst <[email protected]>

* Update bindings/zeebe/command/publish_message_test.go

Signed-off-by: Bernd Verst <[email protected]>

* Update bindings/output_binding.go

Signed-off-by: Bernd Verst <[email protected]>

* Use prepared statement for mysql table creation

Signed-off-by: Bernd Verst <[email protected]>

* Ping is not ping

Signed-off-by: Bernd Verst <[email protected]>

* c'mon linter

Signed-off-by: Bernd Verst <[email protected]>

* Fix MySQL gosec issue

Signed-off-by: Bernd Verst <[email protected]>

* revert mysql to be fixed later

Signed-off-by: Bernd Verst <[email protected]>

Signed-off-by: Bernd Verst <[email protected]>
Signed-off-by: Andrew Duss <[email protected]>
Signed-off-by: Andrew Duss <[email protected]>
Signed-off-by: Andrew Duss <[email protected]>
berndverst
berndverst previously approved these changes Aug 18, 2022
@berndverst
Copy link
Member

Something is not quite right now :)

Please rebase the branch against contrib/master.

@berndverst
Copy link
Member

This PR should not contain all the commits that it does. It should only contain your commits.

Can you please do:

git fetch origin
git rebase origin/master
git push YOURREMOTE --force

@mindovermiles262
Copy link
Contributor Author

I think I got the sign off's fixed. Let me know if I didn't..

Copy link
Contributor

@ItalyPaleAle ItalyPaleAle left a comment

Choose a reason for hiding this comment

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

Just two more small changes to the log messages please :)

Makefile Outdated Show resolved Hide resolved
Makefile Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.