Skip to content
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

🌱 Update Dockerfile #591

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

🌱 Update Dockerfile #591

wants to merge 1 commit into from

Conversation

Mionsz
Copy link

@Mionsz Mionsz commented Nov 10, 2024

What this PR does / why we need it:

Update Dockerfile, add basic labels and speed-up the build process:

  • Dockerfile compliant first line metadata
  • use curl instead of git for repository download
  • use build-time bash shell with -exo pipefail for error catching
  • add proper labeling of resulting image
  • further improve image size by reducing layers number using one RUN command.
  • missing ENTRYPOINT added
  • missing USER added
  • other minor improvements

Update Dockerfile, add basic labels and speed-up the build process:
- use curl instead of git for repository download
- use build-time bash shell with '-exo pipefail' for error catching
- add proper labeling of resulting image
- further improve image size by reducing layers number using one RUN command.
- other minor improvements

Signed-off-by: Miłosz Linkiewicz <[email protected]>

USER ironic
WORKDIR /
ENTRYPOINT ["/bin/bash"]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The entrypoint does not look right.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is also probably not needed

LABEL org.opencontainers.image.title="Metal3 Ironic Container"
LABEL org.opencontainers.image.description="Container image to run OpenStack Ironic as part of Metal³"
LABEL org.opencontainers.image.documentation="https://github.com/metal3-io/ironic-image/blob/main/README.md"
LABEL org.opencontainers.image.version="v26.0.1"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good call. We'd need to update our releasing documentation to bump this (I expect the next version to be 26.1.0 or 27.0.0). CC @elfosardo

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should not have anything but latest in main tho

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is ok for released branches, as Dmitry said probably need to add this when we cut a new branch
in main we should just have latest

@dtantsur
Copy link
Member

Many reasonable changes here, but I'd slightly prefer several pull requests so that we can consider them separately.

A couple of comments (+ requesting input from Riccardo)

@@ -7,25 +9,33 @@ ARG BASE_IMAGE=quay.io/centos/centos:stream9
FROM $BASE_IMAGE AS ironic-builder

ARG IPXE_COMMIT_HASH=119c415ee47aaef2717104fea493377aa9a65874
ARG MAKEFLAGS="-j100"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A 100 jobs, I don't think thats reasonable default.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no need to set MAKEFLAGS at all

LABEL org.opencontainers.image.url="https://github.com/metal3-io/ironic-image"
LABEL org.opencontainers.image.title="Metal3 Ironic Container"
LABEL org.opencontainers.image.description="Container image to run OpenStack Ironic as part of Metal³"
LABEL org.opencontainers.image.documentation="https://github.com/metal3-io/ironic-image/blob/main/README.md"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Documentation would maybe better point to our user-guide?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, let's point to the user guide

@tuminoid
Copy link
Member

/ok-to-test

@metal3-io-bot metal3-io-bot added the ok-to-test Indicates a non-member PR verified by an org member that is safe to test. label Nov 11, 2024
@elfosardo
Copy link
Member

thanks for the patch, appreciate the improvements
I agree with Dmitry though, this change should be split in multiple PRs

@metal3-io-bot metal3-io-bot added the needs-rebase Indicates that a PR cannot be merged because it has merge conflicts with HEAD. label Dec 3, 2024
@metal3-io-bot
Copy link
Contributor

PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-rebase Indicates that a PR cannot be merged because it has merge conflicts with HEAD. ok-to-test Indicates a non-member PR verified by an org member that is safe to test.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants