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

[GH-658] Add golang-ci lint #712

Merged
merged 29 commits into from
Feb 1, 2021

Conversation

carantunes
Copy link
Contributor

@carantunes carantunes commented Nov 8, 2020

Summary

This PR copies most of the golangci-lint configuration from mattermost-community/mattermost-plugin-autolink#108 into this repo. This helps with a consistent code style and should improve code quality over time.

Most changes are straightforward and just style wise. I did comment on the more tricky ones.

Ticket Link

Fixes #658

@carantunes carantunes requested a review from jfrerich as a code owner November 8, 2020 20:05
@mattermod
Copy link
Contributor

Hello @carantunes,

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.

@carantunes carantunes force-pushed the GH-658_add-golang-ci-lint branch from b1f687b to a291787 Compare November 8, 2020 20:11
@hanzei hanzei self-requested a review November 9, 2020 09:57
@hanzei hanzei added 2: Dev Review Requires review by a core committer 3: QA Review Requires review by a QA tester Work In Progress Not yet ready for review labels Nov 9, 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.

This is amazing work 🎉 Thanks for going through all of these files.

Almost all changes look good. There is one issue with a http header. See my comment below.

I would be fine if you comment out the failing linter and we merge this PR as it is.

Comment on lines -209 to -216
# sd is an easier-to-type alias for server-debug
.PHONY: sd
sd: server-debug

# server-debug builds and deploys a debug version of the plugin for your architecture.
# Then resets the plugin to pick up the changes.
.PHONY: server-debug
server-debug: server-debug-deploy reset
Copy link
Collaborator

Choose a reason for hiding this comment

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

@jfrerich Just to double check: Are we fine with removing these commands?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@hanzei I did all the requested changes. Only missing clarification on this point. Should I revert this changes? Copied the makefile from autolink as indicated in the issue.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should keep and investigate these options. They were implemented some time ago, but the idea being that we could save time by only building the appropriate architecture.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Well, technically the are stored in Git and saved there. 1/5 that consistency is more important that the build features.

build/custom.mk Outdated Show resolved Hide resolved
server/jiratracker/tracker.go Outdated Show resolved Hide resolved
server/user_test.go Outdated Show resolved Hide resolved
@codecov-io
Copy link

codecov-io commented Nov 21, 2020

Codecov Report

Merging #712 (e398d5e) into master (776692e) will increase coverage by 0.07%.
The diff coverage is 45.65%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #712      +/-   ##
==========================================
+ Coverage   32.38%   32.46%   +0.07%     
==========================================
  Files          52       52              
  Lines        5373     5357      -16     
==========================================
- Hits         1740     1739       -1     
+ Misses       3433     3417      -16     
- Partials      200      201       +1     
Impacted Files Coverage Δ
server/atlassian_connect.go 0.00% <0.00%> (ø)
server/auth_token.go 3.75% <0.00%> (ø)
server/http.go 26.34% <0.00%> (ø)
server/info.go 55.26% <0.00%> (+2.76%) ⬆️
server/instance_cloud.go 0.00% <0.00%> (ø)
server/instance_server.go 7.14% <0.00%> (ø)
server/instances.go 29.70% <0.00%> (-0.26%) ⬇️
server/stats.go 10.65% <0.00%> (-0.18%) ⬇️
server/user_cloud.go 0.00% <0.00%> (ø)
server/user_server.go 6.81% <0.00%> (+1.15%) ⬆️
... and 29 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 776692e...f47bf95. Read the comment docs.

server/kv.go Outdated Show resolved Hide resolved
@carantunes carantunes changed the title [WIP][GH-658] Add golang-ci lint [GH-658] Add golang-ci lint Nov 21, 2020
@jasonblais
Copy link
Contributor

@jfrerich kind reminder to help with a review of this PR

Copy link
Contributor

@jfrerich jfrerich left a comment

Choose a reason for hiding this comment

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

Amazing amount of work :) Thanks, @Adovenmuehle!

@hanzei, I think we should keep the makefile commands and investigate them. We should consider adding them to the starter-template plugin.

Comment on lines -209 to -216
# sd is an easier-to-type alias for server-debug
.PHONY: sd
sd: server-debug

# server-debug builds and deploys a debug version of the plugin for your architecture.
# Then resets the plugin to pick up the changes.
.PHONY: server-debug
server-debug: server-debug-deploy reset
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should keep and investigate these options. They were implemented some time ago, but the idea being that we could save time by only building the appropriate architecture.

@hanzei hanzei requested a review from DHaussermann December 14, 2020 21:24
@hanzei hanzei removed the 2: Dev Review Requires review by a core committer label Dec 14, 2020
@hanzei
Copy link
Collaborator

hanzei commented Dec 14, 2020

@DHaussermann Maybe this is a good PR to test in the next review process

@DHaussermann
Copy link

/update-branch

@mattermod
Copy link
Contributor

Error trying to update the PR.
Please do it manually.

@DHaussermann
Copy link

Thanks @carantunes for this contribution. I have successfully build and deployed the plugin. I have also done some brief regression testing around core functionality Connect/disconnect create and attach issue etc.. and found no issues.

@hanzei regarding additional testing, this would get alot of coverage to catch potential regression when I do the release testing once Jira 3.0.1 is ready and we have a version bump PR.

Do you feel the risk if fairly low here and we can merge this in before the 3.0.1. release? If so, maybe next steps would be to resolve the conflicts and do one more quick round of testing to get this merged in.

@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
Copy link
Collaborator

hanzei commented Jan 15, 2021

@DHaussermann I highly doubt we will release a 3.0.1 from master. There have been a lot of changes on master since 3.0.0. If we release a 3.0.0, I would expect it to be a released from a separate branch with cherry-picked commits.

Hence, I think it's fine to merge this PR with low amount of testing and rely on the 3.1.0 release testing to catch any bugs.

@carantunes Could you please merge master into your branch?

@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
Copy link
Collaborator

hanzei commented Jan 26, 2021

@DHaussermann Given that 3.0.1 will be not cut from master would be fine merging merging this PR as it is?

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
I did some significant regression testing here #712 (comment)

There is no need to delay the merge to master

Any other regression risk will get covered in the next round of release testing.

Yes @hanzei please merge.

Huge thanks to @carantunes for this enhancement!

@DHaussermann DHaussermann added 4: Reviews Complete All reviewers have approved the pull request and removed 3: QA Review Requires review by a QA tester Lifecycle/1:stale labels Jan 28, 2021
@hanzei hanzei added this to the v3.1.0 milestone Feb 1, 2021
@hanzei hanzei merged commit 8639943 into mattermost:master Feb 1, 2021
@hanzei
Copy link
Collaborator

hanzei commented Feb 1, 2021

Awesome work @carantunes 🚀

@carantunes
Copy link
Contributor Author

Thank you! 🥳 Happy to help again if something comes up!

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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Integrate GolangCI-Lint
7 participants