-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Truncate mode bits for z/OS file permissions #2023
Conversation
Thanks @aguibert! I'm afraid I don't have many z/OS systems knocking around to test this on, so I'll have to take your word for it 😂 Are there any other areas where you're seeing compatibility issues, or is this the only thing? I think we just need to be a little careful about including compatibility fixes for OS environments that we don't currently officially support and which we don't have a plausible CI story. I'm not against this PR, just want to make sure that we're not opening a Pandora's box 😄 |
// Truncate mode bits for z/OS | ||
if ("OS/390".equals(SystemUtils.OS_NAME) || | ||
"z/OS".equals(SystemUtils.OS_NAME) || | ||
"zOS".equals(SystemUtils.OS_NAME) ) { |
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.
Just a thought: is it necessary to decide whether to truncate based on system type, or could we always do it?
Linux is our only supported container OS, so perhaps the way to think about it could be just as ensuring that no matter what unixMode
we get, it's always within the right bounds.
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.
I thought it would be safest to only truncate for z/OS to minimize potential side effects of this PR.
I can't think of any reason a Linux distribution would have out of range mode bits, but I think it would be good to capture such a scenario in the form of an error first so we can dissect what's going on, rather than coercing the mode bits into the allowed range and possibly dealing with harder-to-debug side effects as a result.
hi @rnorth, thanks for having a look at this PR. Let me give a bit more background on the environment where I hit this issue. The scenario where we are hitting this problem is where our test client is running on zOS where we set up an app server, and then we use Testcontainers to spin up databases on a remote Linux machine where Docker is installed. So the container environment here is still Linux, but it just happens to be a Java client running on zOS driving the workload.
Nope, this is the only issue we are facing w/ Testcontainers on zOS. Other than that it works great!
Yea, unfortunately there isn't a good way to publicly test libs with zOS. I'd prefer to contribute this fix back to open source so everyone can benefit from the fix. The alternative is for us to build/maintain an in-house fork of Testcontainers which I'd really like to avoid. |
hi @rnorth, any further thoughts on this bug fix? |
@aguibert sorry for taking ages to get back to you. I'm fine with this, and understand your reasoning, but I'm afraid there's now a merge conflict 😣. |
adf5a28
to
7142997
Compare
resolved the merge conflict so now it should be ready for merge @rnorth |
Thanks @aguibert - finally merged! |
When using Testcontainers on z/OS with the
ImageFromDockerFile.withFileFromFile()
API, file we get an illegal set of mode bits from the OS.Here is an example of my container definition:
We get an error that looks like this:
After adding some debug code to my test, I found out that there is an extra set of mode bits on z/OS files.
In my case, the mode bits are:
Which is outside of the maximum allowed range of:
My proposed solution on this is to simply truncate to the maximum allowed range when running on z/OS, and then setting the default file or dir mode bits (
0100000
or040000
respectively). In this case, it would result in mode bits of: