depsync: add xk6-depsync command #72

roobre · 2023-09-22T12:05:22Z

This PR adds the depsync script we created at xk6-disruptor to the xk6 repository.

xk6-depsync is a simple command that reads the go.mod file from ./go.mod (or another pad provided in os.Args[1]) and looks for dependencies in common with k6 core. For any common package which does not have the same version as k6 core has, it will output to stdout a command that go gets the version that k6 core uses.

As @codebien suggested here, I've added this as a command different to xk6. However this has the downside of not being trivially callable from docker.

This file was born as a script, so there might be a few error-handling corners and best practices that have been cut. LMK if you are okay with those, otherwise I can work on them.

olegbespalov

Generally, it looks good. It's already in my PATH 😅 💪

I left a few minor comments, please mention tool in README, and maybe update targets in Makefile to make it more discoverable.

cmd/xk6-depsync/main.go

roobre · 2023-10-05T14:16:14Z

I've updated the readme with a description of the command and some examples, LMK what you think @olegbespalov!

and maybe update targets in Makefile to make it more discoverable.

I'm not sure I follow you here, to which Makefile are you referring to?

olegbespalov · 2023-10-05T14:19:44Z

@roobre the one from this repository 😅 https://github.com/grafana/xk6/blob/master/Makefile

olegbespalov

Looks good to me, postponing the final approval and merging the decision to the @mstoykov

Also, I feel that @szkiba also could review it 👍

szkiba · 2023-10-05T15:40:13Z

cmd/xk6-depsync/main.go

+		log.Fatalf("opening local go.mod: %v", err)
+	}
+
+	ownDeps, err := dependencies(file)


Consider using https://pkg.go.dev/golang.org/x/[email protected]/modfile#Parse instead of custom parsing logic, it would be a more robust solution....

I've given this a swing in the latest two commits. I wouldn't say I'm against it, but I'm not very convinced either. I don't see the code significantly less complex or significantly shorter either.

LMK what you think about this. If you prefer it this way, I'm happy letting it stay, otherwise I can drop the commits and get back to the original implementation. Your call :)

I didn't recommend it because of the complexity or length of the code. If there is a well-maintained, high-quality library for reading a file format, it is advisable to use it. This way you can avoid that your own parser does not exactly follow the file format specification.

BTW this is just a suggestion, it's up to the code maintainer to decide :)
( @olegbespalov ? )

roobre · 2023-10-06T09:23:56Z

@olegbespalov Ah, got it now, for some reason I thought you were talking about (somehow) adding this to extensions's Makefile and I was very confused 😅

I've added it to the same build target for convenience, LMK if this works for you :)

cmd/xk6-depsync/main.go

codebien

LGTM

cmd/xk6-depsync/main.go

mstoykov · 2023-11-08T12:43:37Z

README.md

@@ -197,6 +197,40 @@ Because the subcommands and flags are constrained to benefit rapid extension pro
 - `XK6_K6_REPO` optionally sets the path to the main k6 repository. This is useful when building with k6 forks.


+## `xk6-depsync`
+
+In addition to `xk6`, this repository also includes the `xk6-depsync` command. `xk6-depsync` can be used to find common dependencies between a given go project, typically a k6 extension, and k6 core. For these common dependencies, `xk6-depsync` will look specifically for those that are required to be on a different version than what k6 uses, and generate a `go get` command to set it to the correct version. This is considered a good chance to minimize the chance of build-time errors and binary compatibility issues.


I do understand that this is related to xk6 (somewhat) and as this it seems like a good idea to keep it in the same repo, but:

Having gone through the code - none of the new code is dependent on xk6 or ... even specific about xk6. But it does add more dependency and makes the README harder to read by making it bigger.

The tool seems to be useful as well for any other projects trying to align their dependencies between different versions.

As such it seems to me it will be more useful to:

Move this whole thing to a separate repo.

the tool to take in another module name as argument.

document here that you can use that repo to sync dependencies.

I agree that from an organizational point of view this tool should be its own module and/or repository.

However, considering that the tool itself is small and, for being a new tool, pretty much unused, I find hard to justify at this point of the tool's lifespan the operational burden of creating a new repository with is corresponding permissions, settings, ci/cd and release pipelines, etc.

I think we can always split the repo (or merge the command) if it starts making sense later on, while benefiting from the simpler operation and discoverability that sharing the repo provides.

Regarding:

But it does add more dependency

I agree and mentioned this here. I'll be okay with reverting 8ae8675 and switch back to the homemade string parser that does not have any external dependency at the cost of perhaps being less conformant with the go.mod spec, if you're okay with that tradeoff.

@roobre I miss this during my reviews, sorry. Why not create a dedicated go.mod file? This is an independent project in the end, being in the same repo doesn't force us to have the same project from Go's point of view.

If we can divide the Go Modules then I share the same feeling as you about having a different repository. A dedicated repo for only one file sounds a bit too much. IMHO.

@codebien No problem, I think creating a different go.mod should be okay. As we already have a go.mod in the root though, do you have any preference about where to put the new one? Would cmd/xk6-depsync/go.mod be okay? Should I also vendor the deps of the new module?

Would cmd/xk6-depsync/go.mod be okay?

Yep, it should be the idiomatic way these days for it. If you can do a quick research to confirm even better.

Should I also vendor the deps of the new module?

I don't think it's required. It's a very simple tool. But I don't have a strong opinion here.

Gave this a try. In order for go build to work, I had to add a go workspace. Otherwise the toolchain does not seem to understand it is a different module:

12:18:03 ~/Staging/xk6 $> go build ./cmd/xk6-depsync main module (go.k6.io/xk6) does not contain package go.k6.io/xk6/cmd/xk6-depsync

I've tested using my fork that go install works as expected:

12:30:39 ~/Staging/xk6/cmd/xk6-depsync $> go install github.com/roobre/xk6/cmd/xk6-depsync@latest go: downloading github.com/roobre/xk6/cmd/xk6-depsync v0.0.0-20231110113016-4811bd1cf67a

@codebien Eager to hear your thoughts on this :)

If you're not positive about the approach (go.work file and all) then perhaps we should go for the route that @mstoykov suggested and just create a new repo for this tool.

Moving the project to another repo also has the benefit of making the tool more general - taking the repo it is supposed to take in as an argument.

Also both tools won't get updated eveyr time the other is

At this point I have to say that :

repos are free.

CI for open source project is free.

copying .github folders takes seconds

I would expect there is even a template repo that makes the above a single click

@roobre I'm fine with moving it. 👍

mstoykov · 2023-11-08T12:43:58Z

README.md

+
+### Trivia
+
+Go versions earlier than 1.21 have been observed to behave unexpectedly in some cases, sometimes ignoring some of the versions specified in the `go get` command, or unexpectedly upgrading/downgrading other dependencies. It is recommended to run the `go get` commands suggested by `xk6-depsync` with Go >= 1.21.


Can you give an example of this?

Yes, we faced this in the xk6-disruptor project. We introduced this code as a script in this commit, which at the time yielded the following command:

go get github.com/spf13/[email protected] github.com/spf13/[email protected] golang.org/x/[email protected] google.golang.org/[email protected]

Running said command with Go 1.21 produced the following changes, which I would pretty much expect:

diff --git a/go.mod b/go.mod index a8cc4d4..1cdab88 100644 --- a/go.mod +++ b/go.mod @@ -4,10 +4,11 @@ go 1.19 require ( github.com/docker/docker v24.0.5+incompatible + github.com/docker/go-connections v0.4.0 github.com/dop251/goja v0.0.0-20230621100801-7749907a8a20 github.com/google/go-cmp v0.5.9 github.com/sirupsen/logrus v1.9.3 - github.com/spf13/cobra v1.5.0 + github.com/spf13/cobra v1.4.0 github.com/testcontainers/testcontainers-go v0.21.0 go.k6.io/k6 v0.46.0 k8s.io/api v0.27.4 @@ -24,7 +25,6 @@ require ( github.com/containerd/containerd v1.7.3 // indirect github.com/cpuguy83/dockercfg v0.3.1 // indirect github.com/docker/distribution v2.8.2+incompatible // indirect - github.com/docker/go-connections v0.4.0 // indirect github.com/docker/go-units v0.5.0 // indirect github.com/emicklei/go-restful/v3 v3.10.1 // indirect github.com/google/pprof v0.0.0-20230207041349-798e818bf904 // indirect @@ -61,7 +61,7 @@ require ( github.com/golang/protobuf v1.5.3 github.com/google/gnostic v0.5.7-v3refs // indirect github.com/google/gofuzz v1.2.0 // indirect - github.com/google/uuid v1.3.0 + github.com/google/uuid v1.3.0 // indirect github.com/imdario/mergo v0.3.15 // indirect github.com/inconshreveable/mousetrap v1.0.0 // indirect github.com/jhump/protoreflect v1.15.1 @@ -78,17 +78,17 @@ require ( github.com/pelletier/go-toml v1.9.5 // indirect github.com/pkg/errors v0.9.1 // indirect github.com/serenize/snaker v0.0.0-20201027110005-a7ad2135616e // indirect - github.com/spf13/afero v1.2.2 // indirect + github.com/spf13/afero v1.1.2 // indirect github.com/spf13/pflag v1.0.5 // indirect github.com/testcontainers/testcontainers-go/modules/k3s v0.21.0 golang.org/x/net v0.11.0 // indirect golang.org/x/oauth2 v0.7.0 // indirect - golang.org/x/sys v0.11.0 // indirect + golang.org/x/sys v0.9.0 // indirect golang.org/x/term v0.9.0 // indirect golang.org/x/text v0.10.0 // indirect golang.org/x/time v0.3.0 // indirect google.golang.org/appengine v1.6.7 // indirect - google.golang.org/grpc v1.57.0-dev + google.golang.org/grpc v1.56.1 google.golang.org/protobuf v1.31.0 gopkg.in/guregu/null.v3 v3.3.0 // indirect gopkg.in/inf.v0 v0.9.1 // indirect

With Go 1.19, however, yielded these changes instead:

diff --git a/go.mod b/go.mod index a8cc4d4..776d9b4 100644 --- a/go.mod +++ b/go.mod @@ -3,32 +3,38 @@ module github.com/grafana/xk6-disruptor go 1.19 require ( - github.com/docker/docker v24.0.5+incompatible + github.com/docker/docker v24.0.6+incompatible + github.com/docker/go-connections v0.4.0 github.com/dop251/goja v0.0.0-20230621100801-7749907a8a20 github.com/google/go-cmp v0.5.9 github.com/sirupsen/logrus v1.9.3 - github.com/spf13/cobra v1.5.0 - github.com/testcontainers/testcontainers-go v0.21.0 + github.com/spf13/cobra v1.4.0 + github.com/testcontainers/testcontainers-go v0.24.0 + github.com/testcontainers/testcontainers-go/modules/k3s v0.24.0 go.k6.io/k6 v0.46.0 - k8s.io/api v0.27.4 - k8s.io/apimachinery v0.27.4 - k8s.io/client-go v0.27.4 + k8s.io/api v0.28.1 + k8s.io/apimachinery v0.28.1 + k8s.io/client-go v0.28.1 sigs.k8s.io/kind v0.13.0 ) require ( + dario.cat/mergo v1.0.0 // indirect github.com/Azure/go-ansiterm v0.0.0-20210617225240-d185dfc1b5a1 // indirect github.com/Microsoft/go-winio v0.6.1 // indirect + github.com/Microsoft/hcsshim v0.11.0 // indirect github.com/bufbuild/protocompile v0.4.0 // indirect github.com/cenkalti/backoff/v4 v4.2.1 // indirect - github.com/containerd/containerd v1.7.3 // indirect + github.com/containerd/containerd v1.7.6 // indirect github.com/cpuguy83/dockercfg v0.3.1 // indirect github.com/docker/distribution v2.8.2+incompatible // indirect - github.com/docker/go-connections v0.4.0 // indirect github.com/docker/go-units v0.5.0 // indirect github.com/emicklei/go-restful/v3 v3.10.1 // indirect + github.com/go-ole/go-ole v1.2.6 // indirect + github.com/google/gnostic-models v0.6.8 // indirect github.com/google/pprof v0.0.0-20230207041349-798e818bf904 // indirect github.com/klauspost/compress v1.16.6 // indirect + github.com/lufia/plan9stats v0.0.0-20211012122336-39d0f177ccd0 // indirect github.com/magiconair/properties v1.8.7 // indirect github.com/moby/patternmatcher v0.5.0 // indirect github.com/moby/sys/sequential v0.5.0 // indirect @@ -38,10 +44,18 @@ require ( github.com/opencontainers/go-digest v1.0.0 // indirect github.com/opencontainers/image-spec v1.1.0-rc4 // indirect github.com/opencontainers/runc v1.1.5 // indirect + github.com/power-devops/perfstat v0.0.0-20210106213030-5aafc221ea8c // indirect + github.com/shirou/gopsutil/v3 v3.23.7 // indirect + github.com/shoenig/go-m1cpu v0.1.6 // indirect + github.com/tklauser/go-sysconf v0.3.11 // indirect + github.com/tklauser/numcpus v0.6.0 // indirect + github.com/yusufpapurcu/wmi v1.2.3 // indirect golang.org/x/exp v0.0.0-20230510235704-dd950f8aeaea // indirect - golang.org/x/mod v0.9.0 // indirect - golang.org/x/tools v0.7.0 // indirect - google.golang.org/genproto v0.0.0-20230410155749-daa745c078e1 // indirect + golang.org/x/mod v0.10.0 // indirect + golang.org/x/tools v0.8.0 // indirect + google.golang.org/genproto/googleapis/rpc v0.0.0-20230525234030-28d5490b6b19 // indirect + gotest.tools/v3 v3.5.1 // indirect + k8s.io/kube-openapi v0.0.0-20230717233707-2695361300d9 // indirect )

Which were pretty unexpected to me, and were pretty much contradicting the command, which asked for google.golang.org/[email protected] and yet yielded:

- google.golang.org/grpc v1.57.0-dev + google.golang.org/grpc v1.57.0

Dockerfile

roobre · 2023-11-20T10:45:20Z

Closing in favor of #73

depsync: add xk6-depsync command

b2111f6

roobre requested review from a team, mstoykov and olegbespalov and removed request for a team September 22, 2023 12:05

olegbespalov reviewed Oct 4, 2023

View reviewed changes

cmd/xk6-depsync/main.go Outdated Show resolved Hide resolved

cmd/xk6-depsync/main.go Show resolved Hide resolved

roobre added 2 commits October 5, 2023 15:49

depsync: fix typo

0be4201

README: add section describing xk6-depsync

f562cbf

roobre requested a review from olegbespalov October 5, 2023 14:16

olegbespalov approved these changes Oct 5, 2023

View reviewed changes

szkiba reviewed Oct 5, 2023

View reviewed changes

roobre added 2 commits October 6, 2023 11:16

depsync: use golang.org/x/mod/modfile for parsing

8ae8675

go.mod: tidy and vendor

6bf8353

roobre force-pushed the depsync branch from a2b7f78 to e168898 Compare October 6, 2023 09:23

Makefile: add xk6-depsync to build target

1a15a46

roobre force-pushed the depsync branch from e168898 to 1a15a46 Compare October 6, 2023 09:24

roobre requested a review from szkiba October 6, 2023 09:24

codebien requested changes Nov 1, 2023

View reviewed changes

cmd/xk6-depsync/main.go Show resolved Hide resolved

roobre requested a review from codebien November 7, 2023 16:01

codebien approved these changes Nov 7, 2023

View reviewed changes

mstoykov reviewed Nov 8, 2023

View reviewed changes

depsync: defer close response body

87a8947

roobre requested a review from mstoykov November 9, 2023 13:22

codebien reviewed Nov 10, 2023

View reviewed changes

Dockerfile Outdated Show resolved Hide resolved

roobre added 3 commits November 10, 2023 12:09

Dockerfile: revert addition of xk6-depsync

c71ad22

fixup! README: add section describing xk6-depsync

622fc4f

fixup! go.mod: tidy and vendor

c9cc4ff

depsync: move command to its own module

43ae42e

roobre force-pushed the depsync branch from e0c10d5 to 43ae42e Compare November 10, 2023 11:25

roobre mentioned this pull request Nov 17, 2023

First implementation grafana/go-depsync#1

Merged

roobre force-pushed the depsync branch 2 times, most recently from 7d61600 to 43ae42e Compare November 20, 2023 10:44

roobre mentioned this pull request Nov 20, 2023

README: suggest using go-depsync to keep dependencies in sync #73

Merged

roobre closed this Nov 20, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

depsync: add xk6-depsync command #72

depsync: add xk6-depsync command #72

roobre commented Sep 22, 2023 •

edited

Loading

olegbespalov left a comment •

edited

Loading

roobre commented Oct 5, 2023

olegbespalov commented Oct 5, 2023

olegbespalov left a comment

szkiba Oct 5, 2023

roobre Oct 6, 2023

szkiba Oct 6, 2023

roobre commented Oct 6, 2023

codebien left a comment

mstoykov Nov 8, 2023

roobre Nov 9, 2023 •

edited

Loading

codebien Nov 9, 2023 •

edited

Loading

roobre Nov 10, 2023 •

edited

Loading

codebien Nov 10, 2023 •

edited

Loading

roobre Nov 10, 2023

roobre Nov 10, 2023

mstoykov Nov 10, 2023

codebien Nov 10, 2023

mstoykov Nov 8, 2023

roobre Nov 9, 2023

roobre commented Nov 20, 2023


		### Trivia

		Go versions earlier than 1.21 have been observed to behave unexpectedly in some cases, sometimes ignoring some of the versions specified in the `go get` command, or unexpectedly upgrading/downgrading other dependencies. It is recommended to run the `go get` commands suggested by `xk6-depsync` with Go >= 1.21.

depsync: add xk6-depsync command #72

depsync: add xk6-depsync command #72

Conversation

roobre commented Sep 22, 2023 • edited Loading

olegbespalov left a comment • edited Loading

Choose a reason for hiding this comment

roobre commented Oct 5, 2023

olegbespalov commented Oct 5, 2023

olegbespalov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roobre commented Oct 6, 2023

codebien left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roobre Nov 9, 2023 • edited Loading

Choose a reason for hiding this comment

codebien Nov 9, 2023 • edited Loading

Choose a reason for hiding this comment

roobre Nov 10, 2023 • edited Loading

Choose a reason for hiding this comment

codebien Nov 10, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roobre commented Nov 20, 2023

roobre commented Sep 22, 2023 •

edited

Loading

olegbespalov left a comment •

edited

Loading

roobre Nov 9, 2023 •

edited

Loading

codebien Nov 9, 2023 •

edited

Loading

roobre Nov 10, 2023 •

edited

Loading

codebien Nov 10, 2023 •

edited

Loading