Skip to content
This repository was archived by the owner on Jul 11, 2023. It is now read-only.

Golang errors pkg #4904

Merged
merged 2 commits into from
Jul 18, 2022
Merged

Golang errors pkg #4904

merged 2 commits into from
Jul 18, 2022

Conversation

allenlsy
Copy link
Contributor

@allenlsy allenlsy commented Jul 15, 2022 •
edited
Loading

Description:

This PR partially resolves #4524

  • Removes "github.com/pkg/errors" package from the repo
  • Wrap errors wherever possible rather than using the string format of the error as part of the message

Testing done:

No functionality changes in this PR. Unit test passes.

Affected area:

Functional Area
New Functionality [ ]
CI System [ ]
CLI Tool [ ]
Certificate Management [ ]
Control Plane [ ]
Demo [ ]
Documentation [ ]
Egress [ ]
Ingress [ ]
Install [ ]
Networking [ ]
Observability [ ]
Performance [ ]
SMI Policy [ ]
Security [ ]
Sidecar Injection [ ]
Tests [ ]
Upgrade [ ]
Other [ ]

Please answer the following questions with yes/no.

  1. Does this change contain code from or inspired by another project?
    • Did you notify the maintainers and provide attribution?

No

  1. Is this a breaking change?

No

  1. Has documentation corresponding to this change been updated in the osm-docs repo (if applicable)?

Not required

@allenlsy allenlsy force-pushed the golang-errors-pkg branch 4 times, most recently from 1c3c523 to 348d1bf Compare July 15, 2022 22:27
@allenlsy
ref: wrapping Golang errors
a35337e 
Signed-off-by: Allen Leigh <allenlsy@gmail.com>
@allenlsy allenlsy force-pushed the golang-errors-pkg branch 3 times, most recently from 7771d79 to 0986241 Compare July 18, 2022 15:57
@codecov-commenter
Copy link

codecov-commenter commented Jul 18, 2022 •
edited
Loading

Codecov Report

Merging #4904 (a9479a0) into main (5f54056) will decrease coverage by 0.00%.
The diff coverage is 29.49%.

❗ Current head a9479a0 differs from pull request most recent head fe638ec. Consider uploading reports for the commit fe638ec to get more accurate results

@@            Coverage Diff             @@
##             main    #4904      +/-   ##
==========================================
- Coverage   68.64%   68.64%   -0.01%     
==========================================
  Files         220      220              
  Lines       15938    15941       +3     
==========================================
+ Hits        10941    10942       +1     
- Misses       4945     4947       +2     
  Partials       52       52
Flag Coverage Δ
unittests 68.64% <29.49%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
cmd/cli/mesh_list.go 68.75% <0.00%> (ø)
cmd/cli/mesh_upgrade.go 64.15% <0.00%> (ø)
cmd/cli/metrics_disable.go 72.04% <0.00%> (ø)
cmd/cli/metrics_enable.go 72.63% <0.00%> (ø)
cmd/cli/namespace_ignore.go 66.66% <0.00%> (ø)
cmd/cli/namespace_list.go 75.75% <0.00%> (ø)
cmd/cli/proxy_get.go 41.17% <0.00%> (ø)
cmd/cli/support_bugreport.go 22.72% <0.00%> (ø)
cmd/cli/uninstall_mesh.go 69.91% <0.00%> (ø)
cmd/cli/verify_connectivity.go 26.31% <0.00%> (ø)
... and 68 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 5f54056...fe638ec. Read the comment docs.

@allenlsy allenlsy marked this pull request as ready for review July 18, 2022 16:13
keithmattix
keithmattix approved these changes Jul 18, 2022
View reviewed changes
Copy link
Contributor

@keithmattix keithmattix left a comment

Choose a reason for hiding this comment

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

Big fan of this change, thanks for doing this! Just as an FYI, this PR will result in linting errors due to errors starting with a capital letter, but contributors can make that change as they work through the code

@trstringer
Copy link
Contributor

@keithmattix, where do you see those linting errors? Doesn't look like they are getting surfaced with the CI run. But if we will introduce linting errors than my vote would be to go with fixing them in this same PR so we don't have to ignore failures.

@keithmattix
Copy link
Contributor

@trstringer apologies, I meant warnings. They come from go-staticcheck (at least in VSCode)
image

@allenlsy
Copy link
Contributor Author

@keithmattix From what I see, those lint errors are from the "Unchanged files with check annotations" section, not because of capitalization. I remember previously we can ignore that. I think this is a new feature in PR. I will fix the errors. But let's confirm if we want to enable this feature.

image

@keithmattix
Copy link
Contributor

To clarify, I'm not suggesting you fix the capitalization. I think the linting errors are safe to ignore for now as well

@allenlsy allenlsy force-pushed the golang-errors-pkg branch from 0986241 to a9479a0 Compare July 18, 2022 16:54
@allenlsy allenlsy marked this pull request as draft July 18, 2022 17:02
@allenlsy allenlsy force-pushed the golang-errors-pkg branch 2 times, most recently from d56941a to fe638ec Compare July 18, 2022 17:10
@allenlsy
ref: use Golang errors rather than pkg/errors
386d41b 
Signed-off-by: Allen Leigh <allenlsy@gmail.com>
@allenlsy allenlsy force-pushed the golang-errors-pkg branch from fe638ec to 386d41b Compare July 18, 2022 17:17
@allenlsy allenlsy marked this pull request as ready for review July 18, 2022 17:23
@trstringer trstringer merged commit 8030047 into openservicemesh:main Jul 18, 2022
@allenlsy allenlsy mentioned this pull request Jul 18, 2022
Closed
@nojnhuh nojnhuh mentioned this pull request Jul 20, 2022
Merged
trstringer pushed a commit that referenced this pull request Jul 20, 2022
@nojnhuh @shalier
@allenlsy @steeling
chore(release): add missing cherry picks (#4932)
4c832d1 
* chore(cmd/cli): Reduce cyclomatic complexity of uninstall command (#4825)

* chore(cmd/cli): Reduce cyclomatic complexity of uninstall command

Separate code into functions to reduce the cyclomatic complexity
And adds unit tests to functions created
Helps unblock #4555

Signed-off-by: Shalier Xia <shalierxia@microsoft.com>
(cherry picked from commit d5d3a25)

* Golang errors pkg (#4904)

This PR partially resolves #4524

* Removes "github.com/pkg/errors" package from the repo
* Wrap errors wherever possible rather than using the string format of the error as part of the message

Signed-off-by: Allen Leigh <allenlsy@gmail.com>
(cherry picked from commit 8030047)

* fix golints and security scan issues (#4915)

fix golints and security for:

1. G112 (ReadHeaderTimeout)
2. prometheus client_go version pinned to bad version

Signed-off-by: Sean Teeling <seanteeling@microsoft.com>

(cherry picked from commit f768f64)

* Fix lints by removing superfluous var type from var instantiation (#4917)

Remove unnecessary var types during instantiation to fix lints

Signed-off-by: Sean Teeling <seanteeling@microsoft.com>
(cherry picked from commit 9e9f712)

Co-authored-by: Shalier Xia <69616256+shalier@users.noreply.github.com>
Co-authored-by: allenlsy <allenlsy@gmail.com>
Co-authored-by: steeling <seanteeling@microsoft.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@shashankram shashankram Awaiting requested review from shashankram

@snehachhabria snehachhabria Awaiting requested review from snehachhabria

@nojnhuh nojnhuh Awaiting requested review from nojnhuh

@draychev draychev Awaiting requested review from draychev

@steeling steeling Awaiting requested review from steeling

3 more reviewers

@trstringer trstringer trstringer approved these changes

@keithmattix keithmattix keithmattix approved these changes

@jaellio jaellio jaellio approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Improve error handling
5 participants
@allenlsy @codecov-commenter @trstringer @keithmattix @jaellio