On 28/08/2026 19:25, Guillaume Tucker wrote: > Hi Nicolas, > > On 28/08/2026 6:29 pm, Nicolas Schier wrote: >> On Mon, Aug 24, 2026 at 12:05:47PM +0200, Guillaume Tucker wrote:
[...] >>> +import tomllib >> >> Have you seen the comment from sashiko? >> >> | Will this unconditional import of tomllib crash the script on startup for >> | users running supported Python versions like 3.9 and 3.10? >> | The kernel's baseline requirement allows Python 3.9.x, but tomllib is only >> | available starting in Python 3.11. >> >> https://sashiko.dev/#/patchset/15e16f175f59ae666036764eb03c40cdf19809c7.1787896890.git.gtuc...@gtucker.io Here's the latest Sashiko review from the v3: https://sashiko.dev/#/patchset/0a88d9d0ebd73a9f6e72399f93705ebc7b49ae9c.1788341513.git.gtucker%40gtucker.io The bump to Python 3.10 is being kept as discussed earlier with Miguel and others. >> (and there are some others...) > > Some of the other comments are a bit bogus, the uid / gid precedence > logic is correct as far as I can tell. It's a matter of convention, > maybe this should just be clarified a bit better in the documentation > (and we may add unit tests at some point...). The comment about > injecting malicious runtime options via the configuration file seems > misled as the user should be able to trust the config file just like > the command line. It's true that the image name itself could be > sanitised for extra safety anyway but that's not something introduced > by the config file. I can do this as a follow-up I guess. > > The comment about a missing whitespace is valid though, and the one > about profiles with integer values of 0 is valid too so I'll get them > fixed in a v3. The comments about UID and GID have been addressed as well as minor typos with missing whitespace etc. The issue about container image name validation will be addressed as a follow-up since it's not introduced by this series. I've prepared some changes with a compliant regex to send on top already but it's quite invasive so I thought it'd be best to get this first series done first. There's a new comment which seems fair enough although that's also partly a matter of CLI options interpretation: | Since there is no --registry command line argument to override or unset the | configuration value, a default registry in .container.toml (like docker.io) | will be blindly prepended to fully-qualified images provided via the -i | option (e.g., quay.io/lib/img). | | This results in invalid image paths like docker.io/quay.io/lib/img and | prevents the use of images from other registries via the command line. This | appears to contradict the documentation's claim that command line options take | precedence over configuration values. I can definitely take this into account as part of the image validation rework since it'll also be able to parse the registry part of a fully-qualified image name. If the registry config option is currently seen as ambiguous, I can just drop it from this series and consider adding it with a more robust implementation then. There's also a new comment about handling TypeError exceptions when a TOML config value has an invalid type (e.g. string instead of int). This isn't really a blocker IMHO but can be handled to improve user experience. In fact I'd consider improving error handling overall and factor-in an earlier comment about when no runtime is found automatically and a more useful message could be provided. So to recap, here's what I would suggest: * keep Python 3.10 for this series, continue discussion about minimum and optional version numbers tree-wide in other threads * drop registry config file option in v4 then add it in a follow-up series along with image name validation and parsing using regex * leave TypeError exceptions as-is in this series, improve user experience and error handling altogether in another follow-up Best wishes, Guillaume
