Address comments on the "rootless CA certs" patch
Address the following problems with #538:
- Correct the shell selection for entrypoint, Ubuntu flavours still need explicit
bashfor variables with dots in their names - Change unhelpful exported variable name (changed from
CACERTtoJRE_CACERTS_PATH) - Change
whichto more-POSIX-compatiblecommand -v - More cleanup
- Explicitely use
TMPDIRwhen available instead of hard-coded/tmp
Ubuntu flavours are broken, I'm on it.
CC @tianon
This is an improvement, yes, thanks :heart:
(definitely like your idea in #573 though, and wonder if, since this rootless functionality hasn't been officially released yet, it could happen sooner and then this is moot?)
wonder if, since this rootless functionality hasn't been officially released yet, it could happen sooner and then this is moot?
That's my thinking too, but since I'm not involved in and not knowledgeable about the release process and its deadlines, I'm offerring both possibilities. #573 still needs to be implemented, tested, documented and experimented with after all and if "July release" is what I think it is, the timeline is rather tight. "rootless" improves on the current process, which is probably valuable by itself, even if we know how it could be improved further.
@karianna Are you aware of anything blocking the review and merge here? I would like to get this in before the July release as QOL improvement.
@sxa and/or @gdams up next to review and merge.
@rassie Apologies for the rebase requirement but I think our release has created conflict for this PR
@karianna rebase done. I hope we can squeeze this patch into the release. Fingers crossed it will help the pain points not exacerbate them.
@karianna I believe you need to re-trigger that one failing Windows test, it's just flaky.
Doh, this should've been part of all those recent updates, right?
Doh, this should've been part of all those recent updates, right?
Probably. I've asked multiple times for a review, but nothing happened. I guess the release happened a couple of days ago and then #612 has been reported, so I thought I'd need to fix that as well.
Doh, this should've been part of all those recent updates, right?
yeah we're working on it, it would appear we've broken some users so I want to get this merged and update the official images soon
@rassie if you can help us resolve #612 in this PR we'll get it merged and kick off a republish
Hold your horses, this build isn't even green yet :)
Hold your horses, this build isn't even green yet :)
yeah just spotted the failing CI, I'll wait until it's green
@rassie if you can help us resolve #612 in this PR we'll get it merged and kick off a republish
This is what is happening, the latest push as of right now should become green. Would still appreciate a closer look at the source from you, I'm quite anxious about breaking more stuff, even though I believe that the tests are solid.
@rassie if you can help us resolve #612 in this PR we'll get it merged and kick off a republish
This is what is happening, the latest push as of right now should become green. Would still appreciate a closer look at the source from you, I'm quite anxious about breaking more stuff, even though I believe that the tests are solid.
This looks good to me, the tests are solid and the changes all seem as expected.
/merge
/approve
Thanks!
/approve
@tianon I'll get this merged and update https://github.com/docker-library/official-images/pull/17249 to include these changes shortly