containers icon indicating copy to clipboard operation
containers copied to clipboard

Address comments on the "rootless CA certs" patch

Open rassie opened this issue 1 year ago • 6 comments

Address the following problems with #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

rassie avatar Jun 03 '24 09:06 rassie

Ubuntu flavours are broken, I'm on it.

rassie avatar Jun 03 '24 09:06 rassie

CC @tianon

karianna avatar Jun 03 '24 09:06 karianna

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

tianon avatar Jun 06 '24 22:06 tianon

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.

rassie avatar Jun 06 '24 22:06 rassie

@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 avatar Jun 26 '24 09:06 rassie

@sxa and/or @gdams up next to review and merge.

karianna avatar Jul 01 '24 01:07 karianna

@rassie Apologies for the rebase requirement but I think our release has created conflict for this PR

karianna avatar Jul 23 '24 07:07 karianna

@karianna rebase done. I hope we can squeeze this patch into the release. Fingers crossed it will help the pain points not exacerbate them.

rassie avatar Jul 23 '24 07:07 rassie

@karianna I believe you need to re-trigger that one failing Windows test, it's just flaky.

rassie avatar Jul 23 '24 08:07 rassie

Doh, this should've been part of all those recent updates, right?

tianon avatar Jul 24 '24 22:07 tianon

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.

rassie avatar Jul 24 '24 22:07 rassie

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

gdams avatar Jul 24 '24 22:07 gdams

@rassie if you can help us resolve #612 in this PR we'll get it merged and kick off a republish

gdams avatar Jul 24 '24 22:07 gdams

Hold your horses, this build isn't even green yet :)

rassie avatar Jul 24 '24 22:07 rassie

Hold your horses, this build isn't even green yet :)

yeah just spotted the failing CI, I'll wait until it's green

gdams avatar Jul 24 '24 22:07 gdams

@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 avatar Jul 24 '24 22:07 rassie

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

gdams avatar Jul 24 '24 22:07 gdams

/merge

gdams avatar Jul 24 '24 22:07 gdams

Approval to merge during the lockdown cycle

Please can two Adoptium PMC members comment /approve?

github-actions[bot] avatar Jul 24 '24 22:07 github-actions[bot]

/approve

gdams avatar Jul 24 '24 22:07 gdams

Thanks!

rassie avatar Jul 24 '24 22:07 rassie

/approve

smlambert avatar Jul 24 '24 22:07 smlambert

@tianon I'll get this merged and update https://github.com/docker-library/official-images/pull/17249 to include these changes shortly

gdams avatar Jul 24 '24 22:07 gdams