-
Notifications
You must be signed in to change notification settings - Fork 445
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Major: should be core22
in both args
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
?