Address comments on the "rootless CA certs" patch #572
Conversation
|
Ubuntu flavours are broken, I'm on it. |
|
CC @tianon |
rassie
force-pushed
the
ca-certs-rework-fixes
branch
2 times, most recently
from
c1d3849
to
4db09df
Compare
|
This is an improvement, yes, thanks ❤️ (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?) |
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
force-pushed
the
ca-certs-rework-fixes
branch
from
4db09df
to
474b3fe
Compare
rassie
force-pushed
the
ca-certs-rework-fixes
branch
from
bd5d0f1
to
6de6f41
Compare
rassie
force-pushed
the
ca-certs-rework-fixes
branch
from
6de6f41
to
089349e
Compare
|
@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. |
|
@rassie Apologies for the rebase requirement but I think our release has created conflict for this PR |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
A block has been put on this Pull Request as this repository is temporarily under a code freeze due to an ongoing release cycle.
If this pull request needs to be merged during the release cycle then please comment /merge and a PMC member will be able to remove the block.
If the code freeze is over you can remove this block by commenting /thaw.
rassie
force-pushed
the
ca-certs-rework-fixes
branch
from
089349e
to
2ae4198
Compare
|
@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. |
rassie
force-pushed
the
ca-certs-rework-fixes
branch
2 times, most recently
from
e93be7c
to
5449635
Compare
|
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. |
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 |
|
Hold your horses, this build isn't even green yet :) |
yeah just spotted the failing CI, I'll wait until it's green |
Address the following problems with adoptium#538: 1. Correct the shell selection for entrypoint, Ubuntu flavours still need explicit `bash` for variables with dots in their names 2. Change unhelpful exported variable name (changed from `CACERT` to `JRE_CACERTS_PATH`) 3. Change `which` to more-POSIX-compatible `command -v` 4. More cleanup 5. Explicitely use `TMPDIR` when available instead of hard-coded `/tmp` 6. Support multi-certificate files (again) 7. Make output less verbose
rassie
force-pushed
the
ca-certs-rework-fixes
branch
from
5449635
to
7c088a6
Compare
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 |
|
Approval to merge during the lockdown cycle Please can two Adoptium PMC members comment |
|
/approve |
|
Thanks! |
|
/approve |
github-actions
bot
dismissed
their stale review
Thank you @gdams and @smlambert for your approvals, this pull request is now approved to merge during release.
|
@tianon I'll get this merged and update docker-library/official-images#17249 to include these changes shortly |
Address the following problems with #538:
bashfor variables with dots in their namesCACERTtoJRE_CACERTS_PATH)whichto more-POSIX-compatiblecommand -vTMPDIRwhen available instead of hard-coded/tmpFixes: #612