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.

Sign up for GitHub

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

Jump to bottom

fix: falco-driver-loader source-only print env vars #2353

Merged
merged 1 commit into from
Aug 31, 2023
Merged

fix: falco-driver-loader source-only print env vars #2353

merged 1 commit into from
Aug 31, 2023

Conversation

steakunderscore
Copy link
Contributor

@steakunderscore steakunderscore commented Jan 12, 2023 •
edited by FedeDP
Loading

What type of PR is this?

/kind bug

Any specific area of the project related to this PR?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Fixes #2352

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

new(falco-driver-loader): --source-only now prints the values as env vars

@poiana poiana requested review from FedeDP and Kaizhe January 12, 2023 17:18
@poiana
Copy link
Contributor

poiana commented Jan 12, 2023

Welcome @steakunderscore! It looks like this is your first PR to falcosecurity/falco 🎉

@poiana poiana added the size/L label Jan 12, 2023
@ggilligan12
Copy link

endorse

@jasondellaluce
Copy link
Contributor

/milestone 0.34.0

@poiana poiana added this to the 0.34.0 milestone Jan 12, 2023
@FedeDP
Copy link
Contributor

FedeDP commented Jan 13, 2023 •
edited
Loading

Hi! Thanks for this PR!

echo " --source-only skip execution and allow sourcing in another script"

This is from the help message of falco-driver-loader. I assume we don't want to print anything in that case; but i fully agree with your idea; we might add a new subcmd, like --print-env ?

@steakunderscore
Copy link
Contributor Author

hmm.. I think this diff actually stems from my lack of understanding of bash sourcing my comment on #2352.

I'd be happy to convert this to a --print-env env flag if you guys think this would be useful? If not, feel free to close this without merging.

@FedeDP
Copy link
Contributor

FedeDP commented Jan 13, 2023

I think it is pretty useful to add the --print-env :)
I'd also print TARGET_ID like https://github.com/falcosecurity/falco/blob/master/scripts/falco-driver-loader#L314 does!

@leogr
Copy link
Member

leogr commented Jan 13, 2023

I like it. I see some failing tests, any clue? 🤔

@steakunderscore
Copy link
Contributor Author

I'll get this cleaned up. I'll also take a look at the tests.

@FedeDP
Copy link
Contributor

FedeDP commented Jan 16, 2023

I don't think tests are actually your fault; we are having some weird errors lately :/

@leogr
Copy link
Member

leogr commented Jan 23, 2023

hmm.. I think this diff actually stems from my lack of understanding of bash sourcing my comment on #2352.

I'd be happy to convert this to a --print-env env flag if you guys think this would be useful? If not, feel free to close this without merging.

Hey @steakunderscore

My vote goes for --print-env!

PS
We are close to the Falco release (by the end of this month). If you can't update the PR in time, I will move it to the next milestone.

@leogr
Copy link
Member

leogr commented Jan 26, 2023

/milestone 0.35.0

@poiana poiana modified the milestones: 0.34.0, 0.35.0 Jan 26, 2023
@poiana
Copy link
Contributor

poiana commented Jun 2, 2023

Stale issues rot after 30d of inactivity.

Mark the issue as fresh with /remove-lifecycle rotten.

Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

Provide feedback via https://github.com/falcosecurity/community.

/lifecycle rotten

@leogr
Copy link
Member

leogr commented Jun 22, 2023

I did not notice that --print-env change request was already addressed, so this should be ready for review.

cc @FedeDP @Andreagit97 @therealbobo

@leogr
Copy link
Member

leogr commented Jun 22, 2023

/remove-lifecycle rotten

@therealbobo
Copy link
Contributor

Left a comment but for the rest LGTM! 😄

@FedeDP
Copy link
Contributor

FedeDP commented Jun 22, 2023

LGTM too; i'd wait for Falco patch release 0.35.1 to be out, before going on with this; great refactor though, much more readable script!

FedeDP
FedeDP approved these changes Aug 31, 2023
View reviewed changes
Copy link
Contributor

@FedeDP FedeDP left a comment

Choose a reason for hiding this comment

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

/approve

@poiana poiana added the lgtm label Aug 31, 2023
@poiana
Copy link
Contributor

poiana commented Aug 31, 2023

LGTM label has been added.

Git tree hash: 7981fe2566489ad317956375788434454a2a2d91

Andreagit97
Andreagit97 approved these changes Aug 31, 2023
View reviewed changes
Copy link
Member

@Andreagit97 Andreagit97 left a comment

Choose a reason for hiding this comment

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

/approve
Thank you andò sorry for the delay!

@Andreagit97
Copy link
Member

/reopen

@poiana poiana reopened this Aug 31, 2023
@poiana
Copy link
Contributor

poiana commented Aug 31, 2023

@Andreagit97: Reopened this PR.

In response to this:

/reopen

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/test-infra repository.

@steakunderscore @Andreagit97
fix(scripts): falco-driver-loader add print env
8859874 
Fixes falcosecurity#2352

Needed to refactor the target_id code paths to allow this to be used in
env printing and sourcing.

Signed-off-by: hjenkins <[email protected]>
@poiana poiana removed the lgtm label Aug 31, 2023
@poiana poiana requested review from Andreagit97 and FedeDP August 31, 2023 09:21
@Andreagit97
Copy link
Member

rebased to fix the CI

FedeDP
FedeDP approved these changes Aug 31, 2023
View reviewed changes
Copy link
Contributor

@FedeDP FedeDP left a comment

Choose a reason for hiding this comment

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

/approve

@poiana poiana added the lgtm label Aug 31, 2023
@poiana
Copy link
Contributor

poiana commented Aug 31, 2023

LGTM label has been added.

Git tree hash: fc82e5c7f360cf45041dbfe788a5a856530ad4d3

@poiana
Copy link
Contributor

poiana commented Aug 31, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Andreagit97, FedeDP, steakunderscore

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@poiana poiana merged commit 63ba159 into falcosecurity:master Aug 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@FedeDP FedeDP FedeDP approved these changes

@Kaizhe Kaizhe Awaiting requested review from Kaizhe

@Andreagit97 Andreagit97 Awaiting requested review from Andreagit97

Assignees

@FedeDP FedeDP

@Andreagit97 Andreagit97

Labels
approved dco-signoff: yes kind/bug lgtm release-note size/L
Projects
None yet
Milestone
0.36.0
Development

Successfully merging this pull request may close these issues.

falco-driver-loader --source-only does nothing
8 participants
@steakunderscore @poiana @ggilligan12 @jasondellaluce @FedeDP @leogr @therealbobo @Andreagit97