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

fix!: docker authentication setup #2727

Merged
merged 1 commit into from
Aug 16, 2024

Conversation

stevenh
Copy link
Collaborator

@stevenh stevenh commented Aug 9, 2024

Check and return errors in the process of determining authentication configs so that unexpected failures don't occur latter in the process of build an image or creating a container.

BuildOptions will now return an empty result on error instead of an incomplete one, to ensure that consumers don't use partial data.

Fix builds with different config or override environments failing when the authentication configuration changes, which was introduced by #2646.

Report errors from GetRegistryCredentials calls to avoid unexpected failures latter on in the authentication process.

Split out the functionality to read a Dockerfile from an io.Reader into ExtractImagesFromReader, as required when processing from a tar archive.

Deprecated function ContainerRequest.GetAuthConfigs will now panic if an error occurs, so that callers understand that an failure occured.

BREAKING CHANGE Add support for determining the required authentication in when building an image from a ContextArchive, this requires ContextArchive support io.Seeker.

@stevenh stevenh requested a review from a team as a code owner August 9, 2024 15:43
Copy link

netlify bot commented Aug 9, 2024

Deploy Preview for testcontainers-go ready!

Name Link
🔨 Latest commit 8c34bcb
🔍 Latest deploy log https://app.netlify.com/sites/testcontainers-go/deploys/66bcecea3cd3590008cfd6fd
😎 Deploy Preview https://deploy-preview-2727--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 configuration.

Copy link
Collaborator

@mdelapenya mdelapenya left a comment

Choose a reason for hiding this comment

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

LGTM although I'd like to discuss a bit on some part of the code. PLMK your thoughts on my comments 🙏

container.go Show resolved Hide resolved
container.go Show resolved Hide resolved
container.go Show resolved Hide resolved
container.go Show resolved Hide resolved
docker_auth.go Show resolved Hide resolved
@stevenh stevenh force-pushed the fix/docker-auth branch 2 times, most recently from 8bfca8b to 4f525d3 Compare August 14, 2024 17:27
Check and return errors in the process of determining authentication
configs so that unexpected failures don't occur latter in the process
of build an image or creating a container.

BuildOptions will now return an empty result on error instead of an
incomplete one, to ensure that consumers don't use partial data.

Fix builds with different config or override environments failing when
the authentication configuration changes, which was introduced by testcontainers#2646.

Report errors from GetRegistryCredentials calls to avoid unexpected
failures latter on in the authentication process.

Split out the functionality to read a Dockerfile from an io.Reader into
ExtractImagesFromReader, as required when processing from a tar archive.

Deprecated function ContainerRequest.GetAuthConfigs will now panic if
an error occurs, so that callers understand that an failure occurred.

Remove unused parameter t from prepareRedisImage.

BREAKING CHANGE Add support for determining the required authentication
in when building an image from a ContextArchive, this requires ContextArchive
support io.Seeker.
@mdelapenya mdelapenya self-assigned this Aug 16, 2024
@mdelapenya mdelapenya added the breaking change Causing compatibility issues. label Aug 16, 2024
Copy link
Collaborator

@mdelapenya mdelapenya left a comment

Choose a reason for hiding this comment

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

LGTM, thanks again for your contributions 🙇

@mdelapenya mdelapenya merged commit 5024e26 into testcontainers:main Aug 16, 2024
112 checks passed
mdelapenya added a commit that referenced this pull request Aug 16, 2024
* main:
  fix: config via environment (#2725)
  fix(redpanda): race condition on port check (#2692)
  fix: logging restart (#2697)
  fix!: docker authentication setup (#2727)
  chore: improve error wrapping (#2720)
  chore: run make tests in verbose mode (#2734)
  chore(deps): bump github.com/docker/docker from 27.1.0+incompatible to 27.1.1+incompatible (#2733)
  fix(kafka): port race on start (#2696)
  docs: fix broken doc tags (#2732)
  fix: nginx request failures (#2723)
  fix(compose): container locking (#2722)
  fix(wait): log test timeout (#2716)
  chore: increase timeout values (#2719)
  chore: remove unused parameters (#2721)
  chore(mockserver): silence warning about internal port (#2730)
  feat(wait): skip internal host port check (#2691)
@stevenh stevenh deleted the fix/docker-auth branch September 2, 2024 18:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change Causing compatibility issues.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants