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

Remove deprecated outputs datadog and kafka #2081

Merged
merged 2 commits into from
Jul 1, 2021
Merged

Conversation

mstoykov
Copy link
Contributor

@mstoykov mstoykov commented Jul 1, 2021

No description provided.

@mstoykov mstoykov added this to the v0.34.0 milestone Jul 1, 2021
@mstoykov mstoykov requested review from imiric, codebien and na-- July 1, 2021 07:31
@codecov-commenter
Copy link

codecov-commenter commented Jul 1, 2021

Codecov Report

Merging #2081 (3dc32a7) into master (cc0361c) will decrease coverage by 0.03%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2081      +/-   ##
==========================================
- Coverage   72.13%   72.10%   -0.04%     
==========================================
  Files         181      180       -1     
  Lines       14289    14243      -46     
==========================================
- Hits        10308    10270      -38     
+ Misses       3353     3348       -5     
+ Partials      628      625       -3     
Flag Coverage Δ
ubuntu 72.04% <0.00%> (-0.04%) ⬇️
windows 71.72% <0.00%> (-0.07%) ⬇️

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

Impacted Files Coverage Δ
cmd/outputs.go 0.00% <0.00%> (ø)
lib/testutils/minirunner/minirunner.go 78.26% <0.00%> (-2.90%) ⬇️
lib/executor/vu_handle.go 95.32% <0.00%> (+0.93%) ⬆️

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 cc0361c...3dc32a7. Read the comment docs.

cmd/outputs.go Outdated Show resolved Hide resolved
cmd/outputs.go Outdated Show resolved Hide resolved
@@ -10,11 +10,12 @@ require (
github.com/andybalholm/brotli v1.0.2
github.com/dop251/goja v0.0.0-20210427212725-462d53687b0d
github.com/fatih/color v1.11.0
github.com/gedex/inflector v0.0.0-20170307190818-16278e9db813 // indirect
Copy link
Member

Choose a reason for hiding this comment

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

Why do we have additions to go.mod when we're removing a dependency? 😕

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This now specifies the version that was required by the removed dependency.
As go always get the lowest possible version, when you remove a dependency that was requiring the highest possible version it will now require something different, which is likely not what you want, so it needs to now require that specific version in go.mod in order for the dependency version to not change

Copy link
Member

Choose a reason for hiding this comment

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

So right now (and probably before), the versions of these indirect dependencies the k6 vendors are not the ones that were required by the actual direct dependencies' go.mod files, right? If so, it's probably not a big deal, but we should probably clean this up now or when we update the direct dependencies of k6? We don't have a good reason for the current state, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the versions of the inderect dependencies were probably all added as case like
The general case I think is, we have:

k6 -> A -> C(v1.2.0)
k6 -> B -> C(v1.3.0)

we dropped dependency B, and if this isn't added we will now get C v1.2.0 as that is the highest version required.

So go mod adds an indirect dependency to C v1.3.0, so we don't change the version of something we didn't intend to.
If at a later point we update A and it requires C v1.4.0 it will be that version that will be used, and from my quick test it will also remove the indirect dependency in go.mod(as it's no longer changing what versions we will get).
If we drop A (in this case that would be api2go ;) ) we will probably also drop the indirect dependencies as well.

So we can clean whenever and change the dependency version, which seems like a way to get something broken. Or just when updating/dropping dependency it will fix itself

Copy link
Contributor Author

Choose a reason for hiding this comment

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

example: do go get google.golang.org/grpc and see how it drops the indirect dependency on xerrors

Co-authored-by: na-- <n@andreev.sh>
@mstoykov mstoykov requested review from na-- July 1, 2021 10:33
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, love to see the negative diff stat. :)

I would've preferred to leave this for the end of the v0.34.0 cycle to keep master working until right before release, but it's not a big deal.

@mstoykov
Copy link
Contributor Author

mstoykov commented Jul 1, 2021

I do prefer to do it now, so we can actually update kafka extension output to depend on version of k6 without that k6 depending on the kafka extension output

@na--
Copy link
Member

na-- commented Jul 1, 2021

Yeah, dropping them ASAP is probably going to be cleaner, and xk6 users shouldn't be affected, since by default they use the latest k6, right? So... :shipit:

@mstoykov mstoykov merged commit aa1fd6a into master Jul 1, 2021
@mstoykov mstoykov deleted the removeDeprecatedOutputs branch July 1, 2021 13:31
@na-- na-- added the breaking change for PRs that need to be mentioned in the breaking changes section of the release notes label Sep 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change for PRs that need to be mentioned in the breaking changes section of the release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants