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

Allow to create a container file from a Transferable (#3814) #3815

Merged
merged 17 commits into from Apr 8, 2022
Merged

Allow to create a container file from a Transferable (#3814) #3815

merged 17 commits into from Apr 8, 2022

Conversation

thomasdarimont
Copy link
Contributor

No description provided.

@thomasdarimont thomasdarimont force-pushed the GH-3814-Allow-container-files-from-Transferable branch from edc3565 to 2fe1746 Compare April 20, 2021 08:28
@thomasdarimont
Copy link
Contributor Author

@bsideup I just rebased this on current master.

@thomasdarimont
Copy link
Contributor Author

The test-failure seems to come from the API compatibility checks:

Execution failed for task ':testcontainers:japicmp'.
> A failure occurred while executing JApicmp check comparing [testcontainers.jar] with [testcontainers-1.15.2.jar]
   > Detected binary changes between testcontainers.jar and testcontainers-1.15.2.jar. See failure report at file:///home/runner/work/testcontainers-java/testcontainers-java/core/build/reports/japi.html

@thomasdarimont
Copy link
Contributor Author

@kiview this is the PR :)

kiview
kiview requested changes May 21, 2021
View reviewed changes
Copy link
Member

@kiview kiview left a comment

Choose a reason for hiding this comment

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

The testcontainers:japicmp of course makes sense since this extends the API.
I am not sure if I am a fan of this implicit ordering that is part of the implementation now, WDYT @bsideup @rnorth?

core/src/main/java/org/testcontainers/containers/GenericContainer.java Show resolved Hide resolved
core/src/test/java/org/testcontainers/containers/GenericContainerTest.java Show resolved Hide resolved
@bsideup
Copy link
Member

bsideup commented May 21, 2021

@kiview need to have a deeper look, but one thing I can say for sure is that public API should not be changed (yay, japicmp! :D)

@kiview kiview mentioned this pull request Feb 22, 2022
Merged
@kiview kiview added this to the next milestone Mar 8, 2022
@kiview kiview mentioned this pull request Mar 24, 2022
Closed
@kiview
Copy link
Member

kiview commented Apr 7, 2022

The failing Netlify jobs originate from the fact, that Netlify tries to build from the branch, rather than from the merged PR (as GHA does). So this branch is still using the old mkdocs version, that pulls in the breaking change from jinja2. Won't be an issue after the merge.

@dajudge dajudge force-pushed the GH-3814-Allow-container-files-from-Transferable branch from 52a5c4b to a5b695f Compare April 7, 2022 13:23
kiview
kiview reviewed Apr 8, 2022
View reviewed changes
core/src/main/java/org/testcontainers/images/builder/Transferable.java
@@ -78,4 +89,8 @@ default void transferTo(TarArchiveOutputStream tarArchiveOutputStream, final Str
default String getDescription() {
return "";
}

default void updateChecksum(Checksum checksum) {
Copy link
Member

Choose a reason for hiding this comment

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

Can we just remove this, to fix japicmp error?

Copy link
Member

Choose a reason for hiding this comment

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

What does it say?

Default methods should be fine, and, worst case scenario, we could just allowlist it

Copy link
Member

Choose a reason for hiding this comment

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

This is the report:
image

This is the explanation from Oracle docs on binary compatibility:

Adding a default method, or changing a method from abstract to default, does not break compatibility with pre-existing binaries, but may cause an IncompatibleClassChangeError if a pre-existing binary attempts to invoke the method.

Copy link
Member

Choose a reason for hiding this comment

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

hmm, of is clearly false positive (isn't a new default method, but static)

updateChecksum can be allow-listed too 👍

core/src/main/java/org/testcontainers/images/builder/Transferable.java
Comment on lines +16 to +18
static Transferable of(String string) {
return of(string.getBytes(StandardCharsets.UTF_8));
}
Copy link
Member

Choose a reason for hiding this comment

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

This is the other change leading to japicmp error. I am not sure what is the desired pattern to avoid this.

Copy link
Member

Choose a reason for hiding this comment

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

wait, what? Added static method makes japicmp report it as binary incompatible change? o_O What does it say exactly?

Copy link
Member

Choose a reason for hiding this comment

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

See the other comment, it fails because of METHOD_NEW_DEFAULT rule violation. But I can't say if this is a FP in this case.

bsideup
bsideup approved these changes Apr 8, 2022
View reviewed changes
Copy link
Member

@bsideup bsideup left a comment

Choose a reason for hiding this comment

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

🎉

@kiview kiview merged commit d93dcea into testcontainers:master Apr 8, 2022
@kiview kiview mentioned this pull request Apr 8, 2022
Closed
@kiview kiview mentioned this pull request May 16, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dajudge dajudge dajudge left review comments

@kiview kiview kiview approved these changes

@bsideup bsideup bsideup approved these changes

@rnorth rnorth Awaiting requested review from rnorth

Assignees
No one assigned
Labels
type/enhancement
Projects
None yet
Milestone
1.17.0
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@thomasdarimont @bsideup @kiview @dajudge