-
Notifications
You must be signed in to change notification settings - Fork 974
chore(docker): improve debian Dockerfile structure and optimize base image setup #3738
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
base: master
Are you sure you want to change the base?
Conversation
|
Welcome @rauldsl! It looks like this is your first PR to falcosecurity/falco 🎉 |
af7200e to
3c01df9
Compare
Signed-off-by: Raul Leite <sp4wn.root@gmail.com>
Signed-off-by: Raul Leite <sp4wn.root@gmail.com>
Signed-off-by: Raul Leite <sp4wn.root@gmail.com>
da1f4b2 to
5f26301
Compare
Signed-off-by: Raul Leite <sp4wn.root@gmail.com>
Signed-off-by: Raul Leite <sp4wn.root@gmail.com>
Signed-off-by: Raul Leite <sp4wn.root@gmail.com>
5f26301 to
04e236e
Compare
|
Release note: |
|
Hey @rauldsl Thanks for this PR. Out of curiosity, may I ask why you didn't use the default PR template and used a custom one? |
@leogr |
Signed-off-by: Raul Leite <sp4wn.root@gmail.com>
|
@leogr Is anything pending on my end regarding this PR? |
Yes, the comments I left above are not actually resolved ( #3738 (review) ). We need |
@rauldsl any update on this? |
Signed-off-by: Raul Leite <sp4wn.root@gmail.com>
f4b2b68 to
41ccadd
Compare
Done, as you requested. |
|
/release-note-none |
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.
Pull request overview
This PR refactors three Dockerfiles to improve structure and maintainability by consolidating environment variable declarations and removing duplicate package installations. The changes are primarily organizational with the goal of improving code clarity without altering runtime behavior.
Key Changes
- Consolidated multiple ENV declarations into single multi-line ENV statements with proper quoting
- Converted some hardcoded values to ARG variables for better parameterization
- Optimized package installation commands by removing duplicates (e.g.,
ca-certificatesin falco-debian)
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| docker/falco/Dockerfile | Consolidated ENV declarations, added ARG variables for HOST_ROOT and HOME, optimized download logic with ARCH variable, and added URL-encoded version handling |
| docker/falco-debian/Dockerfile | Consolidated ENV declarations, added ARG variables for configuration values, added DEBIAN_FRONTEND setting, and removed duplicate ca-certificates package |
| docker/driver-loader-buster/Dockerfile | Consolidated ENV declarations into a single multi-line statement with proper quoting |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
leogr
left a comment
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.
In addition to CoPilot's findings, I have added some minor fixes.
IMO, these are the missing changes required before approving this PR.
Co-authored-by: Leonardo Grasso <me@leonardograsso.com> Signed-off-by: Raul Leite <sp4wn.root@gmail.com>
Co-authored-by: Leonardo Grasso <me@leonardograsso.com> Signed-off-by: Raul Leite <sp4wn.root@gmail.com>
Signed-off-by: Leonardo Grasso <me@leonardograsso.com>
|
LGTM label has been added. DetailsGit tree hash: 0b2299e167d50a730bc972b1c2b7155797d6d12e |
irozzo-1A
left a comment
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.
/lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: irozzo-1A, rauldsl, sgaist The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
ekoops
left a comment
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 left a comment!
|
|
||
| ENV FALCO_VERSION=${FALCO_VERSION} | ||
| ENV VERSION_BUCKET=${VERSION_BUCKET} | ||
| ENV DEBIAN_FRONTEND=noninteractive |
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.
Please don't set DEBIAN_FRONTEND as environment variable, as this will persist at runtime. If you want to use it, you can inline it in the apt command.
What type of PR is this?
/kind cleanup
/kind bug
Any specific area of the project related to this PR?
/area build
What this PR does / why we need it:
This PR removes duplicated package installation blocks inside the Falcon Dockerfile and centralizes environment variables into a single, consistent location.
Which issue(s) this PR fixes:
Fixes #
(If there is no issue linked, leave it empty or remove the line.)
Special notes for your reviewer:
The modifications do not introduce functional changes to Falco’s behavior; they strictly improve the Dockerfile structure and maintainability.
All duplicated package sections were consolidated without altering execution order or runtime behavior.
Does this PR introduce a user-facing change?: