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

Drop github.com/pkg/errors and fix error_code detection #1951

Merged
merged 9 commits into from
Apr 6, 2021

Conversation

na--
Copy link
Member

@na-- na-- commented Apr 5, 2021

In the spirit of spring cleaning and #1933 😅

This also makes some of the changes that #1660 did, though in a slightly different way, so we should probably try to port the rest of them.

@na-- na-- requested review from mstoykov and imiric April 5, 2021 18:21
Copy link
Contributor

@mstoykov mstoykov left a comment

Choose a reason for hiding this comment

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

LGTM, but I would prefer to get the tests from #1660 ported, before we merge this

lib/netext/httpext/error_codes.go Show resolved Hide resolved
@na-- na-- added this to the v0.32.0 milestone Apr 6, 2021
Base automatically changed from golangcilint-update to master April 6, 2021 08:15
@na-- na-- force-pushed the drop-pkg-errors-dependency branch from 0777398 to f995bbb Compare April 6, 2021 09:19
@codecov-io
Copy link

codecov-io commented Apr 6, 2021

Codecov Report

Merging #1951 (74bbe0a) into master (3515db5) will increase coverage by 0.04%.
The diff coverage is 58.40%.

❗ Current head 74bbe0a differs from pull request most recent head bd83527. Consider uploading reports for the commit bd83527 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1951      +/-   ##
==========================================
+ Coverage   71.26%   71.30%   +0.04%     
==========================================
  Files         181      183       +2     
  Lines       14256    14274      +18     
==========================================
+ Hits        10159    10178      +19     
+ Misses       3478     3476       -2     
- Partials      619      620       +1     
Flag Coverage Δ
ubuntu 71.24% <55.04%> (+0.02%) ⬆️
windows 70.90% <58.03%> (+0.02%) ⬆️

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

Impacted Files Coverage Δ
api/v1/group.go 89.06% <0.00%> (ø)
cloudapi/api.go 23.86% <0.00%> (ø)
cloudapi/errors.go 60.00% <ø> (ø)
cmd/cloud.go 4.83% <ø> (ø)
cmd/login_cloud.go 0.00% <ø> (ø)
cmd/run.go 12.01% <0.00%> (+0.05%) ⬆️
cmd/scale.go 0.00% <ø> (ø)
converter/har/converter.go 61.17% <0.00%> (-0.73%) ⬇️
js/bundle.go 92.15% <0.00%> (ø)
js/modules/k6/data/data.go 66.66% <ø> (ø)
... and 31 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 3515db5...bd83527. Read the comment docs.

@na-- na-- changed the title Drop the github.com/pkg/errors dependency and fix some lint issues Drop github.com/pkg/errors and fix error_code detection Apr 6, 2021
@na-- na-- requested a review from mstoykov April 6, 2021 09:25
mstoykov
mstoykov previously approved these changes Apr 6, 2021
Copy link
Contributor

@mstoykov mstoykov left a comment

Choose a reason for hiding this comment

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

LGTM

But the test added in 9fac9ca breaks until d14d24d

@na--
Copy link
Member Author

na-- commented Apr 6, 2021

Yes, I know, I deliberately didn't fix it as a part of the cherry-pick. I'll probably squash this PR when merging it in master, but I'll leave the commits as they are here in the PR, so the iterative issue fixing and refactoring process is clear to anyone that wants to debug it in the future, but we won't have any issues with git bisect or something like that.

imiric
imiric previously approved these changes Apr 6, 2021
Copy link
Contributor

@imiric imiric left a comment

Choose a reason for hiding this comment

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

LGTM, I like the more descriptive error messages. Just left a couple of tiny nitpicks.

js/modules/k6/html/html.go Outdated Show resolved Hide resolved
@na-- na-- dismissed stale reviews from imiric and mstoykov via bd83527 April 6, 2021 15:22
mstoykov
mstoykov previously approved these changes Apr 6, 2021
@na-- na-- merged commit e5d4d82 into master Apr 6, 2021
@na-- na-- deleted the drop-pkg-errors-dependency branch April 6, 2021 15:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants