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

Update libs #2092

Merged
merged 9 commits into from
Jul 14, 2021
Merged

Update libs #2092

merged 9 commits into from
Jul 14, 2021

Conversation

mstoykov
Copy link
Contributor

No description provided.

Mostly some fixes and performance improvements from what we use.
It seems that now protoparse will emit warnings for unused imports after
we add WarningReporter, but it does it for all VUs which needs to be
fixed before it's implemented.
Fixes support for double quotes in the query
@codecov-commenter
Copy link

codecov-commenter commented Jul 13, 2021

Codecov Report

Merging #2092 (0d90e0c) into master (04c8b1f) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2092   +/-   ##
=======================================
  Coverage   72.11%   72.11%           
=======================================
  Files         180      180           
  Lines       14249    14249           
=======================================
  Hits        10275    10275           
  Misses       3348     3348           
  Partials      626      626           
Flag Coverage Δ
ubuntu 72.00% <ø> (-0.03%) ⬇️
windows 71.74% <ø> (+0.02%) ⬆️

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

Impacted Files Coverage Δ
output/cloud/output.go 81.15% <0.00%> (-0.58%) ⬇️
js/runner.go 82.40% <0.00%> (+0.53%) ⬆️

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 04c8b1f...0d90e0c. Read the comment docs.

Copy link
Member

@na-- na-- left a comment

Choose a reason for hiding this comment

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

LGTM, though can you also update the golang.org/x/ repos, so that @codebien doesn't have to in #2090? Ideally, we shouldn't mix dependency updates with feature PRs

@mstoykov
Copy link
Contributor Author

I did update them to the versions that are in the current 1.16 vendored branch. There are changes that are for go 1.17 that I haven't update to.

@mstoykov
Copy link
Contributor Author

an by a "did update" I mean I did run the commands to do and as nothing happened I checked and there are no changes in the internal-branch.go1.16-vendor branches for neither x/crypto nor x/net

na--
na-- previously approved these changes Jul 13, 2021
Copy link
Member

@na-- na-- left a comment

Choose a reason for hiding this comment

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

Huh, I guess github.com/golang/mock@v1.6.0 depend on the master versions then? 😕

@mstoykov
Copy link
Contributor Author

it depends on x/tools which does

imiric
imiric previously approved these changes Jul 13, 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, just a couple of questions:

  • Why these specific libs, or are these all that are outdated?
  • Did you use modtools for this?

@mstoykov
Copy link
Contributor Author

@imiric

  • those were the ones outdated which either had something useful updated and didn't require more work or update x/net and co to their latest master (which is still unreleased for go 1.17). But maybe I should update the remaining 2-3 that had very small inconsequential updates as well just to remove them from the list of ones that have updates 🤔 . I would've made additional PRs for the more problematic updates such as cobra
  • I used modtools check but not modtools update as it will update everything at the same time and I want to do it one by one ;)

This adds NO_COLOR support that we are not using
Deprecates v1.0.1 which we already don't use
Improvements to zstd and others that we don't use
@mstoykov mstoykov dismissed stale reviews from imiric and na-- via 0d90e0c July 14, 2021 07:13
@mstoykov mstoykov requested review from imiric and na-- July 14, 2021 07:21
Comment on lines +123 to +125
if noColorExists() {
c.noColor = boolPtr(true)
}
Copy link
Member

Choose a reason for hiding this comment

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

Honestly, the fact that they touch os and the fact that they don't really give you a choice whether you want to respect NO_COLOR or not is more than a bit shitty... I get that it makes the library API slightly easier to use, and it "just works" for the majority of users, but it makes testing things that use this library (like k6) such a PITA... 😭

At this point, I'd happily remove it and replace it with something else, since we moved the end-of-test summary to JS, we use it very sparingly:

rg -g '!vendor' -g '!go.mod' -g '!go.sum' 'fatih/color|color\.'
ui/form.go
27:	"github.com/fatih/color"
50:		if _, err := fmt.Fprintln(w, color.BlueString(f.Banner)+"\n"); err != nil {
60:				displayLabel += " " + color.New(color.Faint, color.FgCyan).Sprint("["+extra+"]")
66:			color.Set(color.FgCyan)
73:			color.Unset()
80:				if _, printErr := fmt.Fprintln(w, color.RedString("- "+err.Error())); printErr != nil {

ui/pb/progressbar.go
28:	"github.com/fatih/color"
34:	colorFaint   = color.New(color.Faint)
35:	statusColors = map[Status]*color.Color{
36:		Interrupted: color.New(color.FgRed),
37:		Done:        color.New(color.FgGreen),

cmd/cloud.go
37:	"github.com/fatih/color"
333:			valueColor := getColor(noColor || !stdoutTTY, color.FgCyan)

cmd/ui.go
35:	"github.com/fatih/color"
89:func getColor(noColor bool, attributes ...color.Attribute) *color.Color {
91:		c := color.New()
96:	c := color.New(attributes...)
102:	c := getColor(noColor, color.FgCyan)
140:	valueColor := getColor(noColor, color.FgCyan)

cmd/login_cloud.go
29:	"github.com/fatih/color"
145:				valueColor := getColor(noColor || !stdoutTTY, color.FgCyan)

Not now, but the next time we do something with colors in k6...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did test and it definitely doesn't work ... hence #2091 was made ;) So this update has no consequences for us ... it appears. I am pretty sure it's because of https://github.com/k6io/k6/blob/04c8b1fcf9a7e8628c1200603d433b404df241ba/cmd/ui.go#L86-L99

Copy link
Member

Choose a reason for hiding this comment

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

Glad to see my workaround continues to work around the issue 🎉 😅 True, we won't support NO_COLOR until we explicitly add support for it, but I'd still prefer that approach 🤷‍♂️

@mstoykov mstoykov merged commit a33b054 into master Jul 14, 2021
@mstoykov mstoykov deleted the updateLibs branch July 14, 2021 08:28
@na-- na-- added this to the v0.34.0 milestone Sep 7, 2021
@na-- na-- mentioned this pull request Sep 8, 2021
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