-
Notifications
You must be signed in to change notification settings - Fork 912
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
fix(scripts): force falco-driver-loader script to try to compile the driver anyway even on unsupported platforms #2219
Conversation
…driver anyway even on unsupported platforms. Signed-off-by: Federico Di Pierro <[email protected]>
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!
LGTM label has been added. Git tree hash: 5a50819d5b1d1d97967a00532f64a7ec3999cac8
|
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.
Just found two nits, otherwise LGTM.
Putting
/hold
in case you want to apply my suggestions
scripts/falco-driver-loader
Outdated
res=$? | ||
if [ $res != 0 ]; then | ||
>&2 echo "Detected an unsupported target system, please get in touch with the Falco community. Trying to compile anyway." | ||
ENABLE_DOWNLOAD= |
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.
ENABLE_DOWNLOAD= | |
ENABLE_DOWNLOAD="no" |
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.
Ehy Leo! It would be great if this worked :D Unfortunately, in the script we check if it is not emtpy:
if [ -n "$ENABLE_DOWNLOAD" ]; then
I wanted to make the patch with the smallest diff possible, therefore i avoided to touch it.
scripts/falco-driver-loader
Outdated
get_target_id | ||
res=$? | ||
if [ $res != 0 ]; then | ||
>&2 echo "Detected an unsupported target system, please get in touch with the Falco community. Trying to compile anyway." |
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.
Trying to compile anyway
is true only if ENABLE_COMPILE
is yes.
Perhaps it is worth printing a different message if both compile&download are disabled, and give up immediately.
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.
That could be a great idea; but i didn't want to embed more logic inside the script :)
If this works for you, i'd leave it as is; otherwise i can make the small patch.
All in all, i am hoping for falco-driver-loader
to be kicked out by falcoctl
sooner or later!
/milestone 0.33.0 |
…target distro could not be fetched. Signed-off-by: Federico Di Pierro <[email protected]> Co-authored-by: Leonardo Grasso <[email protected]>
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 👍
LGTM label has been added. Git tree hash: 554d3d4d92f388432ec27092586ad907aa477ef3
|
/milestone 0.34.0 |
/hold until Falco 0.33.0 is released |
/unhold |
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.
/approve
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: FedeDP, jasondellaluce, leogr The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
What type of PR is this?
/kind cleanup
Any specific area of the project related to this PR?
What this PR does / why we need it:
Currently, when falco-driver-loader is not able to fetch OS_NAME from multiple files under /etc, like
/etc/os-release
, it exit with an error.I think that the best approach is trying to compile the requested driver in any case; consider that
TARGET_ID
is only really used to concatenate the correct name for the driver on the s3 bucket.So, in case we are not able to fetch any TARGET_ID, just fallback at disabling
ENABLE_DOWNLOAD
and go on to compile the driver.This is mostly useful when using
HOST_ROOT
but the/etc/
folder is not a shared volume under it.Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Does this PR introduce a user-facing change?: