First implementation of DockerRegitry module by davidesalerno · Pull Request #1165 · testcontainers/testcontainers-go

davidesalerno · 2023-05-10T13:25:20Z

What does this PR do?

This PR is adding the new Docker Registry module. This module is wrapping the registry image and it will be useful for those who needs to interact with a container registry to check if images are pushed or not after a build (i.e. local integration test of projects like kaniko or buildah).

netlify · 2023-05-10T13:25:28Z

✅ Deploy Preview for testcontainers-go ready!

Name	Link
🔨 Latest commit	`8e54e79`
🔍 Latest deploy log	https://app.netlify.com/sites/testcontainers-go/deploys/649c8e523e6ec70008b63a90
😎 Deploy Preview	https://deploy-preview-1165--testcontainers-go.netlify.app/modules/dockerregistry
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

mdelapenya

Great contribution! Not sure if you already took a look at the existing tests including an authenticated docker registry (please see https://github.com/testcontainers/testcontainers-go/blob/main/docker_auth_test.go#L240), which I think it could be a great addition to this module, something like adding a customizer for WithAuthentication or similar.

Wdyt?

.github/workflows/module-registry.yml

mkdocs.yml

mdelapenya · 2023-05-10T13:34:39Z

Not to mention that we could eventually merge this PR at this state, without the authentication, and create a follow-up PR with that work. At the end of the day, the module won't be available until the next release

sonarqubecloud · 2023-05-10T13:37:19Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.0% Duplication

modules/registry/go.mod

mdelapenya · 2023-06-13T08:15:31Z

Hi @davidesalerno did you take a look at my comment here #1165 (review)?

davidesalerno · 2023-06-13T13:54:08Z

Hi @davidesalerno did you take a look at my comment here #1165 (review)?

Yes, I've just added a customizer to support an authenticated registry but I'm having some issue in testing it due to issu #279.

Could you help me in test that is working making this unit test working?

mdelapenya · 2023-06-13T14:10:58Z

modules/dockerregistry/dockerregistry.go

+	return &DockerRegistryContainer{Container: container}, nil
+}
+
+func WithAuthentication(dockerAuthConfig string) testcontainers.CustomizeRequestOption {


Let's define the expectations on how to use this module: that would a user expect if the registry runs with authentication?

should it be possible to pass an arbitrary htpassw file? See https://github.com/testcontainers/testcontainers-go/blob/main/testdata/auth/htpasswd

should it be possible to load/push images to it?

I'd try to start with a container request like the one in this test: https://github.com/testcontainers/testcontainers-go/blob/main/docker_auth_test.go#L275-L299, allowing customising the auth file.

Then, the test for the module should check that's is posible to log in into that registry.

Does it make sense to you?

Yes it is making sense to me.

Do we want also a realm as a parameter?

I do not have a strong opinion... I think we can start simple, and iterate with a follow-up PR for the realm if you want. Or if you see it important, add it here. What is your preference? At the same time, if you consider it important for users of the module, then we can include it since the beginning.

Signed-off-by: Davide Salerno <dsalerno@redhat.com>

sonarqubecloud · 2023-06-28T19:47:49Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
8 Code Smells

No Coverage information
11.0% Duplication

mdelapenya · 2023-08-02T15:00:06Z

mkdocs.yml

        - modules/pulsar.md
        - modules/redis.md
        - modules/redpanda.md
+        - modules/dockerregistry.md


This entry will be probably located at L63 https://github.com/testcontainers/testcontainers-go/pull/1165/files#diff-98d0f806abc9af24e6a7c545d3d77e8f9ad57643e27211d7a7b896113e420ed2R63

mdelapenya · 2023-08-02T15:01:00Z

.github/dependabot.yml

        interval: monthly
      open-pull-requests-limit: 3
      rebase-strategy: disabled
+    - package-ecosystem: gomod


This entry will be probably located at L93: https://github.com/testcontainers/testcontainers-go/pull/1165/files#diff-dd4fbda47e51f1e35defb9275a9cd9c212ecde0b870cba89ddaaae65c5f3cd28R93

mdelapenya · 2023-08-02T15:01:41Z

modules/dockerregistry/dockerregistry.go

+func RunContainer(ctx context.Context, opts ...testcontainers.ContainerCustomizer) (*DockerRegistryContainer, error) {
+	req := testcontainers.ContainerRequest{
+		Image:        "registry:2",
+		ExposedPorts: []string{"5000:5000/tcp"},


I think we do not want to always force a fixed port for the container:

Suggested change

ExposedPorts: []string{"5000:5000/tcp"},

ExposedPorts: []string{"5000/tcp"},

mdelapenya · 2023-08-02T15:03:50Z

modules/dockerregistry/dockerregistry.go

+
+}
+
+// WithData customizer that will retrieve a data directory and mount it inside the registry container


Do we need a Docker mount for it? I wonder if copying the files/dir into the container, using the req.Files attribute would be something to consider.

@eddumelendez is there a preference when creating modules: using mounts or just copying files before the container start? 🤔

The preference is to copy files.

@davidesalerno then I'd advocate for converting all the Mounts into Files 🙏

mdelapenya · 2023-08-02T15:04:37Z

modules/dockerregistry/dockerregistry.go

+// WithImage customizer that will override the registry image used
+func WithImage(image string) testcontainers.CustomizeRequestOption {
+	return func(req *testcontainers.GenericContainerRequest) {
+		req.Image = image
+	}
+}


The core library already exposes a testcontainers.WithImage option, which can be used in the modules.

Suggested change

// WithImage customizer that will override the registry image used

func WithImage(image string) testcontainers.CustomizeRequestOption {

return func(req *testcontainers.GenericContainerRequest) {

req.Image = image

}

}

mdelapenya · 2023-08-02T15:05:04Z

modules/dockerregistry/dockerregistry_test.go

+func TestDockerRegistryWithCustomImage(t *testing.T) {
+	ctx := context.Background()
+
+	container, err := RunContainer(ctx, WithImage("docker.io/registry:latest"))


Suggested change

container, err := RunContainer(ctx, WithImage("docker.io/registry:latest"))

container, err := RunContainer(ctx, testcontainers.WithImage("docker.io/registry:latest"))

mdelapenya · 2023-08-02T15:06:29Z

modules/dockerregistry/dockerregistry_test.go

+	_, cancel := context.WithCancel(context.Background())
+	t.Cleanup(cancel)
+}
+func getRegistryPortAndAddress(t *testing.T, err error, container *DockerRegistryContainer, ctx context.Context) (nat.Port, string) {


I'd create a method at the registry container level, so that any user would be able to get both. Please take a look at how mysql or postgres modules expose the ConnectionString

mdelapenya · 2023-08-02T15:09:01Z

modules/dockerregistry/dockerregistry_test.go

+	ctx := context.Background()
+	wd, err := os.Getwd()
+	assert.NoError(t, err)
+	container, err := RunContainer(ctx, WithAuthentication(wd+"/../../testdata/auth"), WithData(wd+"/../../testdata/data"), WithImage("docker.io/registry:latest"))


I'd copy/duplicate the resources that exist in the core library to the module, to avoid relative paths and decouple the module from the upstream core library just a little bit more.

We could decide later if we want to test that feature in the core or in the module.

mdelapenya · 2023-08-02T15:11:11Z

modules/dockerregistry/tools/tools.go

@@ -0,0 +1,11 @@
+//go:build tools


Please take a look at #1303, which removed the dependency with gotestsum at the package dependency level: it's now a responsibility of our CI to install it, simplifying our dependency tree.

Therefore, this file should be removed

mdelapenya · 2023-08-08T09:46:57Z

.github/workflows/module-dockerregistry.yml

@@ -0,0 +1,56 @@
+name: DockerRegistry module pipeline


Please take a look at #1414: this module file has been removed in favour of an array of modules in the ci.yml workflow.

mdelapenya · 2023-08-08T09:48:46Z

modules/dockerregistry/dockerregistry.go

+
+}
+
+// WithData customizer that will retrieve a data directory and mount it inside the registry container


@davidesalerno then I'd advocate for converting all the Mounts into Files 🙏

mdelapenya · 2024-03-11T20:20:23Z

@davidesalerno we have sent #2341 creating a module for the docker registry. It supersedes this one, so I marked it to close this one.

I'd thank you a lot if you could take a look to that PR and come with your feedback, so it should cover your use case 🙏

Cheers!

davidesalerno requested a review from a team as a code owner May 10, 2023 13:25

davidesalerno force-pushed the docker-registry-module branch 2 times, most recently from 5e5b445 to 98072f6 Compare May 10, 2023 13:31

mdelapenya reviewed May 10, 2023

View reviewed changes

.github/workflows/module-registry.yml Outdated Show resolved Hide resolved

mkdocs.yml Outdated Show resolved Hide resolved

mdelapenya reviewed May 12, 2023

View reviewed changes

modules/registry/go.mod Outdated Show resolved Hide resolved

davidesalerno force-pushed the docker-registry-module branch 4 times, most recently from 68403a3 to 61ff364 Compare June 13, 2023 07:05

mdelapenya reviewed Jun 13, 2023

View reviewed changes

First implementation of DockerRegistry module

7025005

Signed-off-by: Davide Salerno <dsalerno@redhat.com>

davidesalerno force-pushed the docker-registry-module branch from df08171 to 9a5ef0d Compare June 28, 2023 17:14

Added customizer to support registry with authentication

3811c5b

Signed-off-by: Davide Salerno <dsalerno@redhat.com>

davidesalerno force-pushed the docker-registry-module branch from 9a5ef0d to 3811c5b Compare June 28, 2023 17:33

davidesalerno added 3 commits June 28, 2023 21:25

Added WithImage customizer and cleaned up unit tests

83aa322

Signed-off-by: Davide Salerno <dsalerno@redhat.com>

Added additional unit tests

f66f0b9

Signed-off-by: Davide Salerno <dsalerno@redhat.com>

Added unit test for WithAuth KO case

8e54e79

Signed-off-by: Davide Salerno <dsalerno@redhat.com>

davidesalerno requested a review from mdelapenya August 2, 2023 14:34

mdelapenya reviewed Aug 2, 2023

View reviewed changes

mdelapenya reviewed Aug 8, 2023

View reviewed changes

mdelapenya mentioned this pull request Mar 11, 2024

feat: add distribution-registry module #2341

Merged

mdelapenya closed this in #2341 Mar 22, 2024

	ExposedPorts: []string{"5000:5000/tcp"},
	ExposedPorts: []string{"5000/tcp"},


		}

		// WithData customizer that will retrieve a data directory and mount it inside the registry container

	container, err := RunContainer(ctx, WithImage("docker.io/registry:latest"))
	container, err := RunContainer(ctx, testcontainers.WithImage("docker.io/registry:latest"))

Uh oh!

Conversation

davidesalerno commented May 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does this PR do?

Uh oh!

netlify bot commented May 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Deploy Preview for testcontainers-go ready!

Uh oh!

mdelapenya left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mdelapenya commented May 10, 2023

Uh oh!

sonarqubecloud bot commented May 10, 2023

Uh oh!

Uh oh!

mdelapenya commented Jun 13, 2023

Uh oh!

davidesalerno commented Jun 13, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sonarqubecloud bot commented Jun 28, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mdelapenya commented Mar 11, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

davidesalerno commented May 10, 2023 •

edited

Loading

netlify bot commented May 10, 2023 •

edited

Loading