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

Migrate logging to use RPC Logging methods #36

Merged
merged 9 commits into from
May 15, 2020
Merged

Migrate logging to use RPC Logging methods #36

merged 9 commits into from
May 15, 2020

Conversation

kosgrz
Copy link
Collaborator

@kosgrz kosgrz commented Mar 11, 2020

Summary

This PR migrates logging to use RPC Logging methods and improves log messages.

Ticket Link

Fixes #32

@kosgrz kosgrz requested a review from hanzei March 11, 2020 22:55
hanzei
hanzei reviewed Mar 12, 2020
View reviewed changes
server/plugin.go Outdated Show resolved Hide resolved
server/plugin.go Outdated Show resolved Hide resolved
@hanzei hanzei added the 2: Dev Review Requires review by a core committer label Mar 12, 2020
@hanzei
Copy link
Contributor

hanzei commented Mar 12, 2020

/update-branch

mattermod and others added 2 commits March 12, 2020 08:50
hanzei
hanzei reviewed Mar 13, 2020
View reviewed changes
Copy link
Contributor

@hanzei hanzei left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I've noticed that there are a couple of uses of fmt.Print* e.g. https://github.com/mattermost/mattermost-plugin-skype4business/blob/master/server/plugin.go#L113. Would you also please migrate them?

server/plugin.go Outdated
err := p.handleAuthorizeInADD(w, r)
if err != nil {
p.API.LogWarn(err.Error())
http.Error(w, err.Error(), http.StatusBadRequest)
Copy link
Contributor

Choose a reason for hiding this comment

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

💯

server/plugin.go Outdated Show resolved Hide resolved
@kosgrz
* change handleProductType, handleClientID, completeAuthorizeInADD an…
5734804 
…d handleRegisterMeetingFromOnlineVersion to return error
@kosgrz
Copy link
Collaborator Author

kosgrz commented Mar 14, 2020

I've noticed that there are a couple of uses of fmt.Print* e.g. https://github.com/mattermost/mattermost-plugin-skype4business/blob/master/server/plugin.go#L113. Would you also please migrate them?

I've already migrated these uses. Your link redirects to branch master :)

@hanzei hanzei self-requested a review March 16, 2020 18:21
hanzei
hanzei approved these changes Mar 19, 2020
View reviewed changes
Copy link
Contributor

@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.

One non blocking question

server/plugin.go Outdated Show resolved Hide resolved
@hanzei hanzei requested a review from larkox March 19, 2020 13:21
@hanzei hanzei added the 3: QA Review Requires review by a QA tester label Mar 19, 2020
larkox
larkox requested changes Mar 20, 2020
View reviewed changes
Copy link
Contributor

@larkox larkox left a comment

Choose a reason for hiding this comment

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

Thank you for the contribution! LGTM, just two changes.

server/plugin.go Outdated Show resolved Hide resolved
server/plugin.go Outdated Show resolved Hide resolved
@codecov-io
Copy link

codecov-io commented Mar 24, 2020 •
edited
Loading

Codecov Report

Merging #36 into master will increase coverage by 3.40%.
The diff coverage is 40.47%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #36      +/-   ##
==========================================
+ Coverage   60.00%   63.40%   +3.40%     
==========================================
  Files           4        4              
  Lines         545      511      -34     
==========================================
- Hits          327      324       -3     
+ Misses        167      136      -31     
  Partials       51       51
Impacted Files Coverage Δ
server/plugin.go 57.75% <40.47%> (+4.07%) ⬆️

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 3aa9dc6...e2ab834. Read the comment docs.

hanzei
hanzei approved these changes Mar 24, 2020
View reviewed changes
Copy link
Contributor

@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.

one nit pick

server/plugin.go Outdated Show resolved Hide resolved
larkox
larkox requested changes Mar 26, 2020
View reviewed changes
server/plugin.go Outdated Show resolved Hide resolved
server/plugin.go Outdated Show resolved Hide resolved
server/plugin.go Outdated Show resolved Hide resolved
server/plugin.go Outdated Show resolved Hide resolved
server/plugin.go Outdated Show resolved Hide resolved
server/plugin.go Outdated Show resolved Hide resolved
@kosgrz
* use errors.Wrapf instead of fmt.Errorf
3593cb5 
* change handleProfileImage function to return error and int
@hanzei hanzei requested a review from larkox March 27, 2020 08:22
larkox
larkox approved these changes Mar 27, 2020
View reviewed changes
Copy link
Contributor

@larkox larkox left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you for the contribution! 🎉

@hanzei hanzei removed the 2: Dev Review Requires review by a core committer label Mar 27, 2020
@hanzei hanzei requested a review from DHaussermann March 27, 2020 09:27
@hanzei
Copy link
Contributor

hanzei commented Mar 27, 2020

@DHaussermann See mattermost-community/mattermost-plugin-autolink#102 (comment) for testing steps

@hanzei hanzei added this to the v1.0.0 milestone Mar 31, 2020
@hanzei
Copy link
Contributor

hanzei commented Apr 2, 2020

@DHaussermann Or maybe you could test this as part of the release testing

@hanzei
Copy link
Contributor

hanzei commented May 15, 2020

The last build broken because golint now insists in using error as the last return value of a function. @kosgrz I hope you don't mind that I fixed it.

@hanzei
Copy link
Contributor

hanzei commented May 15, 2020

Skipping QA review per https://community.mattermost.com/core/pl/k1js57wa8tnr5qtrzturas7txh

@hanzei hanzei added 4: Reviews Complete All reviewers have approved the pull request and removed 3: QA Review Requires review by a QA tester labels May 15, 2020
@hanzei hanzei merged commit fe70d20 into mattermost-community:master May 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hanzei hanzei hanzei approved these changes

@larkox larkox larkox approved these changes

@DHaussermann DHaussermann Awaiting requested review from DHaussermann

Assignees
No one assigned
Labels
4: Reviews Complete All reviewers have approved the pull request
Projects
None yet
Milestone
v0.2.0
Development

Successfully merging this pull request may close these issues.

Migrate logging to use RPC Logging methods
5 participants
@kosgrz @hanzei @codecov-io @larkox @mattermod