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

Issue-184/integrate-golanglint-only #196

Merged
merged 22 commits into from
Dec 9, 2020
Merged

Conversation

AugustasV
Copy link
Contributor

Summary

#184 Only integrated Golangci-lint. I am not able to fix those linter errors, it's just too much of them ( deadcode, errcheck, golint, gosec and others) only fixed some whitespaces

Ticket Link

Integrate Golangci-lint #184

@mattermod
Copy link
Contributor

Hello @AugustasV,

Thanks for your pull request! A Core Committer will review your pull request soon. For code contributions, you can learn more about the review process here.

@hanzei hanzei added 2: Dev Review Requires review by a core committer Work In Progress Not yet ready for review labels Oct 9, 2020
@codecov-io
Copy link

codecov-io commented Oct 9, 2020

Codecov Report

Merging #196 (6ba595f) into master (50beb11) will increase coverage by 0.25%.
The diff coverage is 22.22%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #196      +/-   ##
==========================================
+ Coverage   23.05%   23.30%   +0.25%     
==========================================
  Files          64       64              
  Lines        2316     2291      -25     
==========================================
  Hits          534      534              
+ Misses       1703     1679      -24     
+ Partials       79       78       -1     
Impacted Files Coverage Δ
server/command/command.go 39.02% <0.00%> (ø)
server/command/create_event.go 0.00% <0.00%> (ø)
server/command/unsubscribe.go 0.00% <0.00%> (ø)
server/mscalendar/auto_respond.go 62.96% <ø> (ø)
server/mscalendar/availability.go 55.55% <ø> (ø)
server/mscalendar/calendar.go 0.00% <0.00%> (ø)
server/mscalendar/event_responder.go 0.00% <0.00%> (ø)
server/mscalendar/filters.go 39.13% <ø> (+3.13%) ⬆️
server/mscalendar/mscalendar.go 100.00% <ø> (ø)
server/mscalendar/settings.go 0.00% <0.00%> (ø)
... and 15 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 50beb11...6ba595f. Read the comment docs.

@hanzei hanzei removed the request for review from mickmister October 9, 2020 17:13
@hanzei hanzei linked an issue Oct 9, 2020 that may be closed by this pull request
@mattermod
Copy link
Contributor

This PR has been automatically labelled "stale" because it hasn't had recent activity.
A core team member will check in on the status of the PR to help with questions.
Thank you for your contribution!

/cc @jasonblais @jfrerich

@jasonblais
Copy link

@hanzei @jfrerich what would be a good way to proceed?

@hanzei
Copy link
Collaborator

hanzei commented Oct 21, 2020

Talked with @AugustasV offline. He will disable the failing linters for now.

@AugustasV
Copy link
Contributor Author

@hanzei sure right now build is successful.

@jasonblais
Copy link

@mickmister kind reminder to help with a review :)

Copy link
Member

@mickmister mickmister left a comment

Choose a reason for hiding this comment

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

Awesome work @AugustasV! I have some questions about the Makefile, and a request to group/order package imports if they are from a mattermost repo.

build/custom.mk Outdated Show resolved Hide resolved
Makefile Show resolved Hide resolved
Makefile Show resolved Hide resolved
Makefile Show resolved Hide resolved
server/command/command.go Show resolved Hide resolved
server/mscalendar/welcome_flow.go Show resolved Hide resolved
Copy link
Member

@mickmister mickmister left a comment

Choose a reason for hiding this comment

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

LGTM thanks @AugustasV!

@mattermod
Copy link
Contributor

This PR has been automatically labelled "stale" because it hasn't had recent activity.
A core team member will check in on the status of the PR to help with questions.
Thank you for your contribution!

/cc @jasonblais @jfrerich @emilyacook

@hanzei hanzei removed 2: Dev Review Requires review by a core committer Lifecycle/1:stale labels Dec 4, 2020
@hanzei
Copy link
Collaborator

hanzei commented Dec 4, 2020

@DHaussermann Not sure what to test for this PR. It mostly contains build changes.

@DHaussermann
Copy link

@AugustasV I don't see any lint errors here but I do see what looks like a change in behavior here. if I just type make I see

➜  mattermost-plugin-mscalendar git:(AugustasV-master) ✗ make
go install github.com/golang/mock/mockgen
mockgen -destination server/jobs/mock_cluster/mock_cluster.go github.com/mattermost/mattermost-plugin-api/cluster JobPluginAPI
mockgen -destination server/mscalendar/mock_mscalendar/mock_mscalendar.go github.com/mattermost/mattermost-plugin-mscalendar/server/mscalendar MSCalendar
mockgen -destination server/mscalendar/mock_welcomer/mock_welcomer.go -package mock_welcomer github.com/mattermost/mattermost-plugin-mscalendar/server/mscalendar Welcomer
mockgen -destination server/mscalendar/mock_plugin_api/mock_plugin_api.go -package mock_plugin_api github.com/mattermost/mattermost-plugin-mscalendar/server/mscalendar PluginAPI
mockgen -destination server/remote/mock_remote/mock_remote.go github.com/mattermost/mattermost-plugin-mscalendar/server/remote Remote
mockgen -destination server/remote/mock_remote/mock_client.go github.com/mattermost/mattermost-plugin-mscalendar/server/remote Client
mockgen -destination server/utils/bot/mock_bot/mock_poster.go github.com/mattermost/mattermost-plugin-mscalendar/server/utils/bot Poster
mockgen -destination server/utils/bot/mock_bot/mock_admin.go github.com/mattermost/mattermost-plugin-mscalendar/server/utils/bot Admin
mockgen -destination server/utils/bot/mock_bot/mock_logger.go github.com/mattermost/mattermost-plugin-mscalendar/server/utils/bot Logger
mockgen -destination server/store/mock_store/mock_store.go github.com/mattermost/mattermost-plugin-mscalendar/server/store Store

and then it stops it does not go on to run unit test and build the binary. There are no issues when I type make all any thoughts on what could cause this change or if it's an issue?

cc @hanzei

@hanzei
Copy link
Collaborator

hanzei commented Dec 7, 2020

@DHaussermann For plugin that have a custom makefile at build/custom.mk make will always run the first command in the custom makefile. This is the expected behaviour in this case. The command you are looking for is make all.

@DHaussermann
Copy link

Thanks @hanzei I figured it may not be an issue that you have to use make all and not just make But it felt odd to me that on master branch this behavior is different. Does this also make sense to you?

@hanzei
Copy link
Collaborator

hanzei commented Dec 7, 2020

Yes. This PR was the one that introduced build/custom.mk.

Copy link

@DHaussermann DHaussermann left a comment

Choose a reason for hiding this comment

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

Tested and passed

No issues will build or deployment.
LGTM!

Thanks @AugustasV for adding this! Much appreciated.

@DHaussermann DHaussermann added 4: Reviews Complete All reviewers have approved the pull request and removed 3: QA Review Requires review by a QA tester labels Dec 8, 2020
@hanzei hanzei merged commit 4a9daba into mattermost:master Dec 9, 2020
@hanzei hanzei added this to the v1.1.0 milestone Dec 9, 2020
@hanzei hanzei mentioned this pull request Sep 27, 2021
@mickmister mickmister mentioned this pull request Oct 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4: Reviews Complete All reviewers have approved the pull request Hacktoberfest hacktoberfest-accepted
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Integrate GolangCI-Lint
8 participants