Init xk6 Docker plugin #1

lxkuz · 2021-03-02T21:00:40Z

The update includes the next docker methods:

docker ps
docker start
docker stop
docker pause
docker unpause
docker inspect
docker logs
docker exec

dgzlopes · 2021-03-05T08:35:59Z

Readme.md

+  / __________ \  |__| \__\ \_____/ .io
+
+  execution: local
+     script: ../xk6-docker/test.js


Nit: The script name it's not the same (test.js->script.js).

dgzlopes · 2021-03-05T08:41:40Z

docker.go

+func (d *Docker) Ps() []string {
+	cli, err := client.NewClientWithOpts(client.FromEnv)
+	if err != nil {
+		panic(err)


I think it's better to return the error. But probably @simskij knows better :)

Yes, I agree that we should return the error here rather than panicking. The docker client erroring should probably not be considered fatal for the whole VU goroutine. 🤔

dgzlopes · 2021-03-05T08:44:24Z

docker.go

+	result := make([]string, 0, len(containers))
+
+	for _, container := range containers {
+		result = append(result, fmt.Sprintf("%s %s %s", container.ID[:10], container.Image, container.Command))


Perhaps it's better to return a struct, instead of a string with the concatenated values.

From my POV it's easier to consume from JS.

I agree with @dgzlopes on this one. If the struct itself is too big/contains a lot of properties we don't care about, I'd suggest remapping it to a bespoke struct with reasonable properties and proper json tags.

Consider a scenario where I want to list my containers and get the command used to launch one named whatever-nginx. I'd then. have to regex parse the string from the JS runtime, which likely won't be that fast. If I got a json object on the other hand, it wouldn't be a big deal if I had to console.log the object myself.

@simskij please take a look at rework commit. I tried to keep the initial interface that came from docker go lib. Example js file shows how to get normal docker ps output now. IMO the benefits of this approach are transparent support of https://github.com/moby/moby/tree/master/client functionality. WDYT?

I definitely agree!

If anything, I think it could end up being too much. I don't want to assume too much docker knowledge from the user. But let's start from here. If this extension takes off and people seem to be confused with how to use the props for some reason, we could always limit it in a future version.

dgzlopes

I'm excited about this extension :)

Left some comments (for @simme primarily). Also, I think you should vendor your deps!

simme · 2021-03-05T09:03:55Z

I'm guessing you meant to mention @simskij :)

dgzlopes · 2021-03-05T09:05:29Z

Oh, sorry @simme. I can't trust GH auto-complete 😄

docker.go

simskij · 2021-03-05T15:39:20Z

docker.go

+func (d *Docker) Ps() []string {
+	cli, err := client.NewClientWithOpts(client.FromEnv)
+	if err != nil {
+		panic(err)


Yes, I agree that we should return the error here rather than panicking. The docker client erroring should probably not be considered fatal for the whole VU goroutine. 🤔

simskij · 2021-03-05T15:39:52Z

Great work! 👏🏼 Added a few comments.

lxkuz · 2021-03-07T08:45:53Z

@simskij @dgzlopes Ok I'm removing WIP label as my PR becomes huge and I would like to wrap it up and continue work in tiny PRs. I reworked it since your last review and added the list of docker commands (listed in PR description).
What issues I got:

docker run seems more difficult, will cover it in the next PR.
I did not vendor my deps, please let me know how should I do it properly.
docker logs works weird, see the example. I guess it connected with the buffer somehow, I don't have an exact explanation.

dgzlopes · 2021-03-07T11:59:30Z

Yup! If we've smaller PRs, it's easier to review them.

To vendor the dependencies, usually, I run go mod tidy and then go mod vendor. Once you've done that, you can add and commit the vendor folder.

simskij

Looks superb! That was really fast of you! 🎉 👍🏼
Left some minor suggestions.

simskij · 2021-03-10T09:04:39Z

containers.go

+func (d *Containers) SetupClient() {
+	cli, err := client.NewClientWithOpts(client.FromEnv)
+	if err != nil {
+		panic(err)
+	}
+	d.Client = cli
+}


Maybe we should allow the user to configure this by supplying a config object as well and not strictly enforce config through env vars?

I'm thinking that if an object is supplied as the argument to the function, we should treat that as a client config object, if not - default to FromEnv. If we decide to go for this, it could be good if we also cover it with a unit test for each of the paths.

So you mean the usage will look like this?

import containers from 'k6/x/docker/containers'; export default function () { const containerId = 'YOUR_CONTAINER_ID'; const env = { .... } containers.setup(env) containers.startContainer(containerId, {attach: true}); }

Yeah, exactly! 👍🏼

simskij · 2021-03-10T09:05:06Z

Readme.md

+
+Result output:
+
+```bash


plain might be preferable here given the weird syntax highlighting of the output?

simskij · 2021-03-10T09:21:32Z

containers.go

+func (d *Containers) ListContainers(options types.ContainerListOptions) ([]types.Container, error) {
+	containers, err := d.Client.ContainerList(context.Background(), options)
+	return containers, err
+}
+
+// StartContainer works as Docker start command
+func (d *Containers) StartContainer(containerID string, options types.ContainerStartOptions) error {
+	return d.Client.ContainerStart(context.Background(), containerID, options)
+}
+
+// StopContainer works as Docker stop command
+func (d *Containers) StopContainer(containerID string) error {
+	// TODO: Add timeout option support
+	timeout := 0 * time.Second
+
+	return d.Client.ContainerStop(context.Background(), containerID, &timeout)
+}
+
+// PauseContainer works as Docker pause command
+func (d *Containers) PauseContainer(containerID string) error {
+	return d.Client.ContainerPause(context.Background(), containerID)
+}
+
+// UnpauseContainer works as Docker unpause command
+func (d *Containers) UnpauseContainer(containerID string) error {
+	return d.Client.ContainerUnpause(context.Background(), containerID)
+}
+
+// Logs works as Docker logs command
+func (d *Containers) Logs(containerID string, options types.ContainerLogsOptions) (string, error) {
+	response, err := d.Client.ContainerLogs(context.Background(), containerID, options)
+	buf := new(bytes.Buffer)
+
+	if err != nil {
+		return "", err
+	}
+
+	buf.ReadFrom(response)
+	return buf.String(), nil
+}
+
+// InspectContainer works as Docker inspect command
+func (d *Containers) InspectContainer(containerID string) (types.ContainerJSON, error) {
+	return d.Client.ContainerInspect(context.Background(), containerID)
+}
+
+// ExecContainer works as Docker exec command
+func (d *Containers) ExecContainer(containerID string, config types.ExecConfig) error {
+	response, err := d.Client.ContainerExecCreate(context.Background(), containerID, config)
+	if err != nil {
+		return err
+	}
+	return d.Client.ContainerExecStart(context.Background(), response.ID, types.ExecStartCheck{})
+}


Now that I think about it, and see it in the context of the other commands, maybe we should omit Container from the method names?

containers.listContainers and containers.unpauseContainer looks a bit excessive in comparison to containers.list and containers.unpause, don't you think? Since they are all available from the containers object, it should be quite obvious what they refer to anyhow.

simskij · 2021-03-10T09:23:56Z

examples/docker-logs.js

+    tail: 20,
+    // defaults: true
+  }
+  const result = containers.logs(containerId, options);


This goes for all of the functions that take an options object: Should we provide some "sane" defaults in this case if a user omits the options object from the call? Or is the docker client already taking care of that?

simskij · 2021-03-10T09:26:16Z

examples/docker-pause.js

+
+export default function () {
+  const containerId = 'YOUR_CONTAINER_ID';
+  containers.pauseContainer(containerId, {attach: true});


Suggested change

containers.pauseContainer(containerId, {attach: true});

containers.pauseContainer(containerId);

implementation does not take any args

simskij · 2021-03-10T09:28:01Z

examples/docker-start.js

+
+export default function () {
+  const containerId = 'YOUR_CONTAINER_ID';
+  containers.startContainer(containerId, {attach: true});


What would attaching even mean in this context? That we block until the container exits?

This interface is wrong, I just remove it and keep containerId only

simskij · 2021-03-10T09:31:44Z

As discussed on slack, I'd also suggest having a look at whether we can generate typescript types for the extension using something like https://github.com/OneOfOne/struct2ts or https://github.com/tkrajina/typescriptify-golang-structs. This would definitely improve the developer experience while writing scripts, regardless if the user is using typescript or not.

simskij · 2021-03-11T18:36:25Z

Yeah, I'll go ahead and merge this and then we can continue in smaller PR:s. 👍🏼

Add go project with k6 extention blank

85d03c8

lxkuz requested a review from simskij March 2, 2021 21:00

lxkuz added 2 commits March 5, 2021 01:21

Fix extension name

6657a38

Add Readme

80de384

lxkuz force-pushed the feat/go-project branch from 11b9a0e to 83df800 Compare March 4, 2021 20:58

Add Docker ps support

cdb1c62

lxkuz force-pushed the feat/go-project branch from 83df800 to cdb1c62 Compare March 4, 2021 21:02

dgzlopes reviewed Mar 5, 2021

View reviewed changes

simskij suggested changes Mar 5, 2021

View reviewed changes

lxkuz added 11 commits March 6, 2021 19:34

Rework ListContainers method

1b5d376

Add StartContainer method

5306609

Add StopContainer method

43f20fa

Move docker ps, stop and start under k6/x/docker/containers namespace

2429219

Extract containers logic into separate file

69b65fc

Add PauseContainer method

d0a2acc

Add UnpauseContainer method

a04063e

Add Docker logs method

00d64e5

Pull client setup to separate method

66fe8e9

Add Docker inspect method

e429482

Add Docker exec method

038c343

lxkuz force-pushed the feat/go-project branch from badab17 to 038c343 Compare March 7, 2021 07:54

Polish examples

34a6208

lxkuz changed the title ~~[WIP] Add go project with k6 extention blank~~ Add go project with k6 extention blank Mar 7, 2021

lxkuz requested review from dgzlopes and simskij March 7, 2021 08:46

Vendor dependencies

88f65fe

lxkuz changed the title ~~Add go project with k6 extention blank~~ Init xk6 Docker plugin Mar 9, 2021

simskij suggested changes Mar 10, 2021

View reviewed changes

lxkuz added 4 commits March 11, 2021 12:02

Exclude 'Container' word from container method names

d419595

Cleanup containers Start

90b994a

Fix containers Pause example

f62d7fc

Polish Readme

b74b0a1

simskij merged commit b969b31 into main Mar 11, 2021

simskij deleted the feat/go-project branch March 11, 2021 18:36

This was referenced Mar 11, 2021

Maybe we should allow the user to configure this by supplying a config object as well and not strictly enforce config through env vars? #2

Open

Keep function calls simple by providing sane defaults #3

Open

Add typescript types for the extension #4

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Init xk6 Docker plugin #1

Init xk6 Docker plugin #1

lxkuz commented Mar 2, 2021 •

edited

dgzlopes Mar 5, 2021

dgzlopes Mar 5, 2021 •

edited

simskij Mar 5, 2021

lxkuz Mar 6, 2021

dgzlopes Mar 5, 2021 •

edited

simskij Mar 5, 2021 •

edited

lxkuz Mar 6, 2021

simskij Mar 10, 2021

dgzlopes left a comment •

edited

simme commented Mar 5, 2021

dgzlopes commented Mar 5, 2021

simskij Mar 5, 2021

simskij commented Mar 5, 2021 •

edited

lxkuz commented Mar 7, 2021 •

edited

dgzlopes commented Mar 7, 2021

simskij left a comment

simskij Mar 10, 2021

lxkuz Mar 11, 2021 •

edited

simskij Mar 11, 2021

simskij Mar 10, 2021

lxkuz Mar 11, 2021

simskij Mar 10, 2021

lxkuz Mar 11, 2021

simskij Mar 10, 2021

simskij Mar 10, 2021

lxkuz Mar 11, 2021

simskij Mar 10, 2021

lxkuz Mar 11, 2021

lxkuz Mar 11, 2021

simskij commented Mar 10, 2021

simskij commented Mar 11, 2021

	containers.pauseContainer(containerId, {attach: true});
	containers.pauseContainer(containerId);


		Result output:

		```bash

Init xk6 Docker plugin #1

Init xk6 Docker plugin #1

Conversation

lxkuz commented Mar 2, 2021 • edited

Choose a reason for hiding this comment

dgzlopes Mar 5, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dgzlopes Mar 5, 2021 • edited

Choose a reason for hiding this comment

simskij Mar 5, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dgzlopes left a comment • edited

Choose a reason for hiding this comment

simme commented Mar 5, 2021

dgzlopes commented Mar 5, 2021

Choose a reason for hiding this comment

simskij commented Mar 5, 2021 • edited

lxkuz commented Mar 7, 2021 • edited

dgzlopes commented Mar 7, 2021

simskij left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lxkuz Mar 11, 2021 • edited

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

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

simskij commented Mar 10, 2021

simskij commented Mar 11, 2021

lxkuz commented Mar 2, 2021 •

edited

dgzlopes Mar 5, 2021 •

edited

dgzlopes Mar 5, 2021 •

edited

simskij Mar 5, 2021 •

edited

dgzlopes left a comment •

edited

simskij commented Mar 5, 2021 •

edited

lxkuz commented Mar 7, 2021 •

edited

lxkuz Mar 11, 2021 •

edited