Migrate logging to use RPC Logging methods

[mo2menelzeiny]

Summary

Migrates logging to use API Logging

Ticket Link

Closes #99

[codecov-io]

Codecov Report

Merging #102 into master will increase coverage by 0.46%.
The diff coverage is 40.00%.

@@            Coverage Diff             @@
##           master     #102      +/-   ##
==========================================
+ Coverage   40.28%   40.74%   +0.46%     
==========================================
  Files           6        6              
  Lines         561      562       +1     
==========================================
+ Hits          226      229       +3     
  Misses        313      313              
+ Partials       22       20       -2     

Impacted Files	Coverage Δ
server/autolinkplugin/plugin.go	67.00% <25.00%> (-1.37%)	⬇️
server/autolinkplugin/config.go	73.61% <100.00%> (+5.11%)	⬆️

---

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 a10f0ee...433c563. Read the comment docs.

[hanzei]

While touching the code it would be great to get some improvements in. Let me know if you are fine refining the code a bit more

[hanzei]

@mo2menelzeiny Regarding your question on #99 (comment): No there are no test that cover these lines of code. FYI: You can see which lines are covered by running make coverage. If you want to add unit test for them, it would be highly appreciated, but this is no requirement for merging this PR.

Manually testing is quite hard, as the failure cases are hard to produce.

[mo2menelzeiny]

@hanzei awesome, I will work on the requested refinements

In regards to the test coverage, I noticed that all the cases where there's a check on an error object are not covered in the unit tests, is this intentional?

Also I started writing some tests for config.go however I have some issues with some coupled dependencies that I'm not able to mock, like for example this line
https://github.com/mattermost/mattermost-plugin-autolink/blob/37ebe2af40d3a79042f94ae23af67ae0df4c27dc/server/autolinkplugin/config.go#L28
is there an easy way to mock the Compile() funciton? because it's depending on Config which is declared in the same scope not passed.
https://github.com/mattermost/mattermost-plugin-autolink/blob/37ebe2af40d3a79042f94ae23af67ae0df4c27dc/server/autolinkplugin/config.go#L21
any ideas on that matter?

[hanzei]

In regards to the test coverage, I noticed that all the cases where there's a check on an error object are not covered in the unit tests, is this intentional?

No that is not intentional, this are just missing tests 😉

[hanzei]

Mocking is one way of testing, another would be to just test the error case. In this case an invalid pattern would cause an error:
https://github.com/mattermost/mattermost-plugin-autolink/blob/37ebe2af40d3a79042f94ae23af67ae0df4c27dc/server/autolink/autolink.go#L81-L83

[hanzei]

Heads up that there is a conflict to resolve

[hanzei]

Nice 👍

[levb]

Nice!

[levb]

@DHaussermann this can use a general smoke test (and maybe checking that at least some errors are actually logged?) - perhaps bundled in with the release test?

[DHaussermann]

@mo2menelzeiny Thanks for this enhancment!

I did some regression testing of this PR. No issues found.

As mentioned above, it seems difficult to hit these cases and see the actual logging occur.
LGTM!

@levb I agree we should merge this.

[mo2menelzeiny]

awesome job guys, thank you!
@hanzei, @levb, @DHaussermann