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

Add golangci-lint, fix golangci-lint issues #208

Merged
merged 4 commits into from
Sep 30, 2020

Conversation

vladimirdotk
Copy link
Contributor

@vladimirdotk vladimirdotk commented Sep 25, 2020

Summary

Ticket Link

Fixes #206

@codecov
Copy link

codecov bot commented Sep 25, 2020

Codecov Report

Merging #208 into master will increase coverage by 0.14%.
The diff coverage is 49.50%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #208      +/-   ##
==========================================
+ Coverage   38.97%   39.11%   +0.14%     
==========================================
  Files          16       16              
  Lines        1578     1585       +7     
==========================================
+ Hits          615      620       +5     
- Misses        895      898       +3     
+ Partials       68       67       -1     
Impacted Files Coverage Δ
server/api.go 0.00% <0.00%> (ø)
server/configuration.go 14.70% <0.00%> (ø)
server/plugin.go 12.63% <15.00%> (ø)
server/command.go 32.77% <18.18%> (-0.10%) ⬇️
server/subscriptions.go 44.44% <25.00%> (-0.62%) ⬇️
server/utils.go 36.84% <33.33%> (ø)
server/webhook/issue.go 81.42% <75.00%> (-0.39%) ⬇️
server/subscription/subscription.go 100.00% <100.00%> (ø)
server/webhook/merge_request.go 88.00% <100.00%> (+0.67%) ⬆️
server/webhook/pipeline.go 85.96% <100.00%> (ø)

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 6d72c82...aa4b0f6. Read the comment docs.

@hanzei hanzei self-requested a review September 26, 2020 07:09
@hanzei hanzei added 2: Dev Review Requires review by a core committer 3: QA Review Requires review by a QA tester labels Sep 26, 2020
Copy link
Collaborator

@hanzei hanzei left a comment

Choose a reason for hiding this comment

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

Great work 👍 Thanks for going through all of these files. A few questions/comments below

build/manifest/main.go Show resolved Hide resolved
server/api.go Outdated Show resolved Hide resolved
server/command_test.go Show resolved Hide resolved
server/command_test.go Show resolved Hide resolved
server/command_test.go Outdated Show resolved Hide resolved
server/gitlab/api.go Outdated Show resolved Hide resolved
server/gitlab/api.go Outdated Show resolved Hide resolved
server/plugin.go Outdated Show resolved Hide resolved
server/subscription/subscription.go Show resolved Hide resolved
@hanzei hanzei added this to the v1.3.0 milestone Sep 26, 2020
build/manifest/main.go Outdated Show resolved Hide resolved
@hanzei
Copy link
Collaborator

hanzei commented Sep 26, 2020

@vladimirdotk One small thing: Would you mind not force pushing to your branch? It makes it harder for the review to only review the changes that happen since the last review. Commits will be squashed anyway on merge. You can find more about the reasoing at https://developers.mattermost.com/blog/submitting-great-prs/#4-avoid-force-pushing.

Copy link
Collaborator

@hanzei hanzei left a comment

Choose a reason for hiding this comment

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

Nice 🚀

Copy link
Contributor

@iomodo iomodo 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 @vladimirdotk

@iomodo iomodo removed the 2: Dev Review Requires review by a core committer label Sep 28, 2020
@hanzei
Copy link
Collaborator

hanzei commented Sep 28, 2020

@DHaussermann Is just requires a general smoke test

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

  • Build the plugin. No issues woth go lint
  • Deployed and briefly smoke tested several slash commands ad well as wenhook delivery and notifications.
  • No issues found.

LGTM!
Thanks @vladimirdotk for this PR!

@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 Sep 29, 2020
@hanzei hanzei merged commit 698f20a into mattermost:master Sep 30, 2020
@hanzei hanzei added the Docs/Not Needed Does not require documentation label Dec 11, 2020
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 Docs/Not Needed Does not require documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Integrate GolangCI-Lint
4 participants