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

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

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 :)

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?

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

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.

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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants