docker: add core22 base #4290
docker: add core22 base #4290
Conversation
Codecov Report
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. @@ Coverage Diff @@
## main #4290 +/- ##
==========================================
+ Coverage 88.99% 89.12% +0.12%
==========================================
Files 296 318 +22
Lines 20208 21171 +963
==========================================
+ Hits 17984 18868 +884
- Misses 2224 2303 +79 see 52 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
This also makes Ubuntu 22.04 and snapcraft/stable the default for the builder and runner image. Moves repeated code from Dockerfile into a script to avoid copypasta.
|
|
||
| # Grab the core snaps (which snapcraft uses as a base) from the stable channel | ||
| # and unpack them in the proper place. | ||
| BASES="core core18 core20 core22" |
There was a problem hiding this comment.
it doesn't make much sense to build a snap with a core18 base if the base of the OCI image is 22.04
There was a problem hiding this comment.
There are many snaps that need core18. Why it is wrong to build core18 with 22.04?
docker/Dockerfile
Outdated
| @@ -58,11 +36,16 @@ FROM ubuntu:$UBUNTU | |||
| COPY --from=builder /snap/core /snap/core | |||
| COPY --from=builder /snap/core18 /snap/core18 | |||
| COPY --from=builder /snap/core20 /snap/core20 | |||
| COPY --from=builder /snap/core20 /snap/core22 | |||
There was a problem hiding this comment.
Major: should be core22 in both args
There was a problem hiding this comment.
Missed that. Thanks. Fixed.
| COPY --from=builder /snap/snapcraft /snap/snapcraft | ||
| COPY --from=builder /snap/bin/snapcraft /snap/bin/snapcraft | ||
|
|
||
| # Generate locale and install dependencies. | ||
| RUN apt-get update && apt-get dist-upgrade --yes && apt-get install --yes snapd sudo locales && locale-gen en_US.UTF-8 | ||
| RUN apt-get update && apt-get -y dist-upgrade && apt-get -y install \ |
There was a problem hiding this comment.
Minor: not sure whether this change is really necessary as it just changes the order of arguments and adds lines
There was a problem hiding this comment.
It is Docker best practices https://docs.docker.com/develop/develop-images/dockerfile_best-practices/#sort-multi-line-arguments
There was a problem hiding this comment.
Yeah, following best practices is a good thing, but it's not mentioned in the commit message and looks unrelated (could be in another commit and PR)
| ARG RISK=edge | ||
| ARG UBUNTU=xenial | ||
| ARG RISK=stable | ||
| ARG UBUNTU=22.04 |
There was a problem hiding this comment.
Minor: In README.md codename of release is used, probably makes sense to stick to codenames and replace with jammy ?
There was a problem hiding this comment.
I rarely remember which release name is which version. I remember that 22.04 is the latest LTS, and I guess that's just what matters here.
|
Thank you for this PR, I've tested it and improved a bit for my needs: https://github.com/VladRassokhin/snapcraft/tree/dockerfile Spotted problem:
|
|
we do not want to change this for legacy reasons, we have however started https://github.com/canonical/snapcraft-rocks as our new initiative |
|
@sergiusens how is that related to rocks exactly? Why it replaces this image which is used for communication with snapcraft servers on non-ubuntu machines? |
make lint?pytest tests/unit?