-
-
Notifications
You must be signed in to change notification settings - Fork 1.8k
Allow to create a container file from a Transferable (#3814) #3815
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
edc3565 to
2fe1746
Compare
|
@bsideup I just rebased this on current master. |
|
The test-failure seems to come from the API compatibility checks: |
|
@kiview this is the PR :) |
kiview
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
core/src/test/java/org/testcontainers/containers/GenericContainerTest.java
Show resolved
Hide resolved
|
@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) |
|
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 |
core/src/main/java/org/testcontainers/containers/GenericContainer.java
Outdated
Show resolved
Hide resolved
core/src/main/java/org/testcontainers/images/builder/Transferable.java
Outdated
Show resolved
Hide resolved
core/src/main/java/org/testcontainers/utility/MountableFile.java
Outdated
Show resolved
Hide resolved
52a5c4b to
a5b695f
Compare
core/src/main/java/org/testcontainers/containers/GenericContainer.java
Outdated
Show resolved
Hide resolved
| return ""; | ||
| } | ||
|
|
||
| default void updateChecksum(Checksum checksum) { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
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 👍
| static Transferable of(String string) { | ||
| return of(string.getBytes(StandardCharsets.UTF_8)); | ||
| } |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
core/src/main/java/org/testcontainers/containers/GenericContainer.java
Outdated
Show resolved
Hide resolved
bsideup
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎉

No description provided.