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.

Sign up for GitHub

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

Jump to bottom

fix: do not override ImageBuildOptions.Labels when building from a Dockerfile #2775

Merged
merged 22 commits into from
Sep 18, 2024
Merged

fix: do not override ImageBuildOptions.Labels when building from a Dockerfile #2775

merged 22 commits into from
Sep 18, 2024

Conversation

rzajac
Copy link
Contributor

@rzajac rzajac commented Sep 9, 2024

Closes #2632.

@rzajac rzajac requested a review from a team as a code owner September 9, 2024 17:32
@netlify Netlify
Copy link

netlify bot commented Sep 9, 2024 •
edited
Loading

✅ Deploy Preview for testcontainers-go ready!

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

stevenh
stevenh requested changes Sep 13, 2024
View reviewed changes
internal/core/labels_test.go Outdated Show resolved Hide resolved
internal/core/labels_test.go Outdated Show resolved Hide resolved
internal/core/labels_test.go Outdated Show resolved Hide resolved
internal/core/labels_test.go Outdated Show resolved Hide resolved
rzajac and others added 4 commits September 15, 2024 00:13
@rzajac @stevenh
Update internal/core/labels_test.go
1f7c060 
Co-authored-by: Steven Hartland <stevenmhartland@gmail.com>
@rzajac @stevenh
Update internal/core/labels_test.go
316bbfe 
Co-authored-by: Steven Hartland <stevenmhartland@gmail.com>
@rzajac @stevenh
Update internal/core/labels_test.go
3b8a6a0 
Co-authored-by: Steven Hartland <stevenmhartland@gmail.com>
@rzajac @stevenh
Update internal/core/labels_test.go
0481053 
Co-authored-by: Steven Hartland <stevenmhartland@gmail.com>
mdelapenya
mdelapenya reviewed Sep 16, 2024
View reviewed changes
Copy link
Member

@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.

Just added a few minor comments regarding previous suggestions by @stevenh (thanks!)

I think, once resolved, we are good to go.

Thanks!

container.go Outdated Show resolved Hide resolved
internal/core/labels.go Outdated Show resolved Hide resolved
internal/core/labels.go Outdated Show resolved Hide resolved
internal/core/labels_test.go Outdated Show resolved Hide resolved
container_test.go Outdated Show resolved Hide resolved
container_test.go Outdated Show resolved Hide resolved
rzajac and others added 6 commits September 16, 2024 16:58
@rzajac @mdelapenya
Update container.go
a638238 
Co-authored-by: Manuel de la Peña <social.mdelapenya@gmail.com>
@rzajac @mdelapenya
Update internal/core/labels.go
4c12f9c 
Co-authored-by: Manuel de la Peña <social.mdelapenya@gmail.com>
@rzajac @mdelapenya
Update container_test.go
f9b95ef 
Co-authored-by: Manuel de la Peña <social.mdelapenya@gmail.com>
@rzajac
Fix testcontainers#2632 - remove given, when, then comments.
19474d4
@rzajac @mdelapenya
Update internal/core/labels.go
e51843b 
Co-authored-by: Manuel de la Peña <social.mdelapenya@gmail.com>
@rzajac
Fix testcontainers#2632 - unit test update.
0f4c7ad
stevenh
stevenh requested changes Sep 17, 2024
View reviewed changes
Copy link
Collaborator

@stevenh stevenh left a comment

Choose a reason for hiding this comment

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

Just two little fixes, then should be good to go.

internal/core/labels.go Outdated Show resolved Hide resolved
internal/core/labels_test.go Outdated Show resolved Hide resolved
rzajac and others added 2 commits September 17, 2024 10:40
@rzajac @stevenh
Update internal/core/labels_test.go
6dd21d2 
Co-authored-by: Steven Hartland <stevenmhartland@gmail.com>
@rzajac
Fix testcontainers#2632 - return error when destination labels map is…
560bc38 
… nil and custom labels are present.
stevenh
stevenh requested changes Sep 17, 2024
View reviewed changes
internal/core/labels.go Outdated Show resolved Hide resolved
internal/core/labels.go Outdated Show resolved Hide resolved
internal/core/labels.go Outdated Show resolved Hide resolved
internal/core/labels_test.go Outdated Show resolved Hide resolved
internal/core/labels_test.go Outdated Show resolved Hide resolved
internal/core/labels_test.go Outdated Show resolved Hide resolved
mdelapenya
mdelapenya reviewed Sep 17, 2024
View reviewed changes
Copy link
Member

@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.

@rzajac I think this PR looks really good, thanks for addressing all our comments and sorry of they took that long 🙏

Once the last final suggestions are addressed, I think we are good to go. In the meantime, it LGTM (will approve once they CI passes too).

rzajac and others added 6 commits September 17, 2024 21:00
@rzajac @stevenh
Update internal/core/labels_test.go
a5ecd0d 
Co-authored-by: Steven Hartland <stevenmhartland@gmail.com>
@rzajac @stevenh
Update internal/core/labels.go
11b9551 
Co-authored-by: Steven Hartland <stevenmhartland@gmail.com>
@rzajac @stevenh
Update internal/core/labels.go
b1aa423 
Co-authored-by: Steven Hartland <stevenmhartland@gmail.com>
@rzajac @stevenh
Update internal/core/labels.go
a2f9393 
Co-authored-by: Steven Hartland <stevenmhartland@gmail.com>
@rzajac @stevenh
Update internal/core/labels_test.go
e126d91 
Co-authored-by: Steven Hartland <stevenmhartland@gmail.com>
@rzajac @stevenh
Update internal/core/labels_test.go
f1f9286 
Co-authored-by: Steven Hartland <stevenmhartland@gmail.com>
@rzajac
Copy link
Contributor Author

rzajac commented Sep 17, 2024

@mdelapenya Glad to be of help!

stevenh
stevenh previously approved these changes Sep 17, 2024
View reviewed changes
Copy link
Collaborator

@stevenh stevenh left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Nice, thanks for doing this. We need to check if the test failures are caused by this and fix if needed.

mdelapenya
mdelapenya reviewed Sep 18, 2024
View reviewed changes
Copy link
Member

@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.

Just one last minute change, regarding how the test is cleaning up the created container.

Right after that, we are good to go, thanks!

container_test.go Outdated Show resolved Hide resolved
@rzajac @mdelapenya
Update container_test.go
ec0d18b 
Co-authored-by: Manuel de la Peña <social.mdelapenya@gmail.com>
mdelapenya
mdelapenya approved these changes Sep 18, 2024
View reviewed changes
Copy link
Member

@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 for your patience during the review 🙇

@mdelapenya mdelapenya self-assigned this Sep 18, 2024
@mdelapenya mdelapenya added the bug An issue with the library label Sep 18, 2024
@mdelapenya mdelapenya changed the title Fix #2632 - ImageBuildOptions.Labels are overwritten fix: do not override ImageBuildOptions.Labels when building from a Dockerfile Sep 18, 2024
@mdelapenya mdelapenya merged commit 31a033c into testcontainers:main Sep 18, 2024
112 checks passed
@mdelapenya mdelapenya mentioned this pull request Sep 20, 2024
Merged
mdelapenya added a commit that referenced this pull request Sep 20, 2024
@mdelapenya
Merge branch 'main' into v1
1a9498b 
* main:
  feat: support databend module (#2779)
  chore: golangci-lint 1.61.0 (#2787)
  fix(mssql): bump Docker image version (#2786)
  fix: handle 127 error code for podman compatibility (#2778)
  fix: do not override ImageBuildOptions.Labels when building from a Dockerfile (#2775)
  feat(mongodb): Wait for mongodb module with a replicaset to finish (#2777)
  fix(postgres): Apply default snapshot name if no name specified (#2783)
@mdelapenya mdelapenya mentioned this pull request Sep 23, 2024
Closed
mdelapenya added a commit to mdelapenya/testcontainers-go that referenced this pull request Sep 23, 2024
@mdelapenya
Merge branch 'main' into catinapoke/main
2150dba 
* main:
  chore: use a much smaller image for testing (testcontainers#2795)
  fix: parallel containers clean race (testcontainers#2790)
  fix(registry): wait for (testcontainers#2793)
  fix: container timeout test (testcontainers#2792)
  docs: document redpanda options (testcontainers#2789)
  feat: support databend module (testcontainers#2779)
  chore: golangci-lint 1.61.0 (testcontainers#2787)
  fix(mssql): bump Docker image version (testcontainers#2786)
  fix: handle 127 error code for podman compatibility (testcontainers#2778)
  fix: do not override ImageBuildOptions.Labels when building from a Dockerfile (testcontainers#2775)
  feat(mongodb): Wait for mongodb module with a replicaset to finish (testcontainers#2777)
  fix(postgres): Apply default snapshot name if no name specified (testcontainers#2783)
  fix: resource clean up for tests and examples (testcontainers#2738)
  ci: add generate for mocks (testcontainers#2774)
  fix: docker config error handling when config file does not exist (testcontainers#2772)
mdelapenya added a commit to mdelapenya/testcontainers-go that referenced this pull request Sep 23, 2024
@mdelapenya
Merge branch 'main' into windows-fixes
dcef9bb 
* main:
  chore: use a much smaller image for testing (testcontainers#2795)
  fix: parallel containers clean race (testcontainers#2790)
  fix(registry): wait for (testcontainers#2793)
  fix: container timeout test (testcontainers#2792)
  docs: document redpanda options (testcontainers#2789)
  feat: support databend module (testcontainers#2779)
  chore: golangci-lint 1.61.0 (testcontainers#2787)
  fix(mssql): bump Docker image version (testcontainers#2786)
  fix: handle 127 error code for podman compatibility (testcontainers#2778)
  fix: do not override ImageBuildOptions.Labels when building from a Dockerfile (testcontainers#2775)
  feat(mongodb): Wait for mongodb module with a replicaset to finish (testcontainers#2777)
  fix(postgres): Apply default snapshot name if no name specified (testcontainers#2783)
  fix: resource clean up for tests and examples (testcontainers#2738)
mdelapenya added a commit that referenced this pull request Sep 26, 2024
@mdelapenya
Merge branch 'main' into dev-tools
dc43e00 
* main: (29 commits)
  fix: template for code generation (#2800)
  fix: update module path (#2797)
  fix: container logging deadlocks (#2791)
  chore: use a much smaller image for testing (#2795)
  fix: parallel containers clean race (#2790)
  fix(registry): wait for (#2793)
  fix: container timeout test (#2792)
  docs: document redpanda options (#2789)
  feat: support databend module (#2779)
  chore: golangci-lint 1.61.0 (#2787)
  fix(mssql): bump Docker image version (#2786)
  fix: handle 127 error code for podman compatibility (#2778)
  fix: do not override ImageBuildOptions.Labels when building from a Dockerfile (#2775)
  feat(mongodb): Wait for mongodb module with a replicaset to finish (#2777)
  fix(postgres): Apply default snapshot name if no name specified (#2783)
  fix: resource clean up for tests and examples (#2738)
  ci: add generate for mocks (#2774)
  fix: docker config error handling when config file does not exist (#2772)
  docs: refine heading badges in README (#2770)
  feat(wait): for file (#2731)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mdelapenya mdelapenya mdelapenya approved these changes

@stevenh stevenh stevenh left review comments

Assignees

@mdelapenya mdelapenya

Labels
bug An issue with the library
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Bug]: ImageBuildOptions.Labels are overwritten
3 participants
@rzajac @stevenh @mdelapenya