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: Include the container name in the reuse hash #1162

Merged

Conversation

0xced
Copy link
Contributor

@0xced 0xced commented Apr 25, 2024

What does this PR do?

This pull request removes the [JsonIgnore] attribute from the ContainerConfiguration.Name property so that the container name becomes part of the reuse hash.

Why is it important?

The rationale behind this change is explained in #1161.

Important

Changing the parts used in the reuse hash will break existing reused containers but the best time to do it is now, while the reuse feature is still at the experimental stage.

Related issues

Closes #1161

How to test this PR

A new unit test (ContainersWithDifferentNamesShouldHaveDifferentHashes) has been added.

Copy link

netlify bot commented Apr 25, 2024

Deploy Preview for testcontainers-dotnet ready!

Name Link
🔨 Latest commit d715a18
🔍 Latest deploy log https://app.netlify.com/sites/testcontainers-dotnet/deploys/662b55eb7bf3d1000850f66e
😎 Deploy Preview https://deploy-preview-1162--testcontainers-dotnet.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.

@0xced 0xced changed the title Use the container name in the reuse hash Include the container name in the reuse hash Apr 25, 2024
Copy link
Collaborator

@HofmeisterAn HofmeisterAn left a comment

Choose a reason for hiding this comment

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

I do not remember, but I thought this was on purpose. However, I think excluding it does not make much sense. IIRC when we initially discussed the feature, we were considering hashing the container response from the Docker Engine API (which contains random names). Nevertheless, LGTM. Could you please update the documentation accordingly?

- [ContainerConfiguration](https://github.com/testcontainers/testcontainers-dotnet/blob/develop/src/Testcontainers/Configurations/Containers/ContainerConfiguration.cs)
- Image
- Entrypoint
- Command
- Environments
- ExposedPorts
- PortBindings
- NetworkAliases
- ExtraHosts
- Labels

@HofmeisterAn HofmeisterAn added enhancement New feature or request breaking change Causing compatibility issues labels Apr 26, 2024
@0xced
Copy link
Contributor Author

0xced commented Apr 26, 2024

Could you please update the documentation accordingly?

Of course! I just pushed d715a18 which updates the documentation.

@HofmeisterAn HofmeisterAn changed the title Include the container name in the reuse hash feat: Include the container name in the reuse hash Apr 26, 2024
@HofmeisterAn HofmeisterAn merged commit 619233a into testcontainers:develop Apr 26, 2024
11 checks passed
@0xced 0xced deleted the feature/reuse-container-name-hash branch April 26, 2024 18:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change Causing compatibility issues enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Enhancement]: Make the container name part of the reuse hash
2 participants