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

feat: support passing registry credentials to the reaper #647

Merged
merged 15 commits into from
Jan 2, 2023

Conversation

mdelapenya
Copy link
Member

@mdelapenya mdelapenya commented Dec 2, 2022

  • feat: support passing registry credentials to the reaper
  • chore: support passing registry creds for the reaper when creating networks
  • chore: support configuring the reaper as functional opts
  • chor: remove unused field

What does this PR do?

This PR is addresses the need of passing a registry credentials string to the Reaper. For that we are refactoring the NewReaper constructor to make it more configurable and, at the same time, deprecating it to avoid incorrect usage when creating a Reaper instance.

For the registry credentials string, we have create a functional option, as we consider it is the Go idiom for providing dynamic configuration. The functional option returns a function that receive a containerOptions, a private struct type with the dynamic fields to configure a reaper. In this PR we are adding the aforementioned RegistryCredentials string and the configurable ImageName string as containerOptions. The latter is used everywhere when creating a Reaper instance and we considered it made sense to include it in this PR, not in a follow-up.

We have created a new private constructor for the reaper, newReaper, which accepts functional options in a variadic manner (as last parameter). Then, the old constructor has been deprecated internally to call the private one, passing the old reaperImageName argument as the corresponding functional option. We have updated the usage of the old reaper to the new, private one.

To set the new functional reaper options, the types allowing setting a reaper (NetworkRequest and ContainerRequest) have been modified:

  • the old fields in those structs have been deprecated, in favor of the new functional options
  • a new field with an array of ContainerOption has been added to the structs, making it possible to configure the reaper in a more dynamic (and advanced) manner.
  • all usages of those fields (mostly in the tests of the library) have been updated to use the new functional opts.
  • we have added tests demonstrating that the processed requests include the settings for the reaper

Why is it important?

We want to fix the credentials bug, but at the same time, limit the configuration of the reaper in a smooth manner, so consumer do not get affect by these transition to a more consistent reaper. You can probably identify this PR with #549 #561 and #633 because of the reaper creation. This PR could establish the basis for a more consistent reaper usage.

Related issues

Follow-ups

We could revisit #561 once this PR is merged. Regarding #633, if this PR gets merged we'll probably need to resolve conflicts, but keep the patterns introduced here for functional options.

@mdelapenya mdelapenya requested a review from a team as a code owner December 2, 2022 08:29
@mdelapenya mdelapenya added the chore Changes that do not impact the existing functionality label Dec 2, 2022
@mdelapenya mdelapenya self-assigned this Dec 2, 2022
@netlify
Copy link

netlify bot commented Dec 23, 2022

Deploy Preview for testcontainers-go ready!

Name Link
🔨 Latest commit 86c6a1b
🔍 Latest deploy log https://app.netlify.com/sites/testcontainers-go/deploys/63b2c0d56aa9ee0009f6166b
😎 Deploy Preview https://deploy-preview-647--testcontainers-go.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

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

docker.go Outdated Show resolved Hide resolved
docker.go Outdated Show resolved Hide resolved
@mdelapenya mdelapenya merged commit 67184ce into testcontainers:main Jan 2, 2023
@mdelapenya mdelapenya added enhancement New feature or request and removed chore Changes that do not impact the existing functionality labels Jan 2, 2023
@mdelapenya mdelapenya deleted the reaper-functional-opts branch January 2, 2023 21:41
mdelapenya added a commit to mdelapenya/testcontainers-go that referenced this pull request Jan 4, 2023
* main: (44 commits)
  feat: support passing registry credentials to the reaper (testcontainers#647)
  fix: close response body in http strategy (testcontainers#718)
  chore: move e2e module to postgres example module (testcontainers#717)
  chore: bump containerd transitive dep in examples (testcontainers#715)
  chore(deps): bump github.com/containerd/containerd from 1.6.12 to 1.6.14 (testcontainers#703)
  chore(deps): bump github.com/compose-spec/compose-go in /modules/compose (testcontainers#710)
  chore: bump testcontainers-go to 0.17.0 in examples (testcontainers#714)
  chore(deps): bump github.com/docker/compose/v2 in /modules/compose (testcontainers#711)
  chore: support running MySQL compose in ARM (testcontainers#712)
  chore: simplify compose replace directives (testcontainers#713)
  chore: add compose module to dependabot (testcontainers#709)
  chore: move compose code to a separate module (testcontainers#650)
  docs: refine onboarding process with quickstart guide (testcontainers#706)
  chore: move redis-specific tests to the example module (testcontainers#701)
  chore: bump transitive dependencies (#527)
  chore: reduce concurrent builds (testcontainers#702)
  chore: add mysql example (testcontainers#700)
  chore(deps): bump google.golang.org/api from 0.104.0 to 0.105.0 (testcontainers#699)
  chore(deps): bump google.golang.org/api in /examples/firestore (testcontainers#683)
  chore(deps): bump cloud.google.com/go/spanner in /examples/spanner (testcontainers#688)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: ReaperImage pull doesn't use RegistryCred for ContainerRequest
2 participants