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

new(falco): add buffer_format_base64 option, deprecate -b #3358

Merged
merged 2 commits into from
Oct 10, 2024
Merged

new(falco): add buffer_format_base64 option, deprecate -b #3358

merged 2 commits into from
Oct 10, 2024

Conversation

LucaGuerra
Copy link
Contributor

What type of PR is this?

Uncomment one (or more) /kind <> lines:

/kind feature

Any specific area of the project related to this PR?

Uncomment one (or more) /area <> lines:

/area engine

What this PR does / why we need it:

Add buffer_format_base64 option which behaves like -b --print-base64, works in the config file or with -o.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

new(falco): add buffer_format_base64 option, deprecate -b

@poiana poiana requested review from incertum and Kaizhe September 30, 2024 16:11
@poiana poiana added the size/S label Sep 30, 2024
@LucaGuerra
Copy link
Contributor Author

/milestone 0.40.0

@poiana poiana added this to the 0.40.0 milestone Sep 30, 2024
@LucaGuerra LucaGuerra changed the title new(falco): add buffer_format_base64 option, deprecate -b wip: new(falco): add buffer_format_base64 option, deprecate -b Sep 30, 2024
@LucaGuerra LucaGuerra force-pushed the new/buffer_format_base64 branch from 7483a14 to c5d9c92 Compare October 1, 2024 08:34
@LucaGuerra LucaGuerra changed the title wip: new(falco): add buffer_format_base64 option, deprecate -b new(falco): add buffer_format_base64 option, deprecate -b Oct 1, 2024
FedeDP
FedeDP reviewed Oct 1, 2024
View reviewed changes
Copy link
Contributor

@FedeDP FedeDP left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM!
Again, i have the same comment as #3362 (comment); wdyt?

@LucaGuerra LucaGuerra force-pushed the new/buffer_format_base64 branch from c5d9c92 to 3f0e1bc Compare October 2, 2024 09:31
@poiana poiana added size/M and removed size/S labels Oct 2, 2024
@LucaGuerra
Copy link
Contributor Author

@FedeDP done!

FedeDP
FedeDP previously approved these changes Oct 7, 2024
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 Oct 7, 2024
@poiana
Copy link
Contributor

poiana commented Oct 7, 2024

LGTM label has been added.

Git tree hash: cf294153c28c96ca5298cc8ececd533dc683b3de

Andreagit97
Andreagit97 previously approved these changes Oct 7, 2024
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

@LucaGuerra LucaGuerra dismissed stale reviews from Andreagit97 and FedeDP via 5a00f2d October 10, 2024 07:41
@LucaGuerra LucaGuerra force-pushed the new/buffer_format_base64 branch from 3f0e1bc to 5a00f2d Compare October 10, 2024 07:41
@poiana poiana removed the lgtm label Oct 10, 2024
@poiana poiana requested review from Andreagit97 and FedeDP October 10, 2024 07:41
sgaist
sgaist previously approved these changes Oct 10, 2024
View reviewed changes
@poiana poiana added the lgtm label Oct 10, 2024
@poiana
Copy link
Contributor

poiana commented Oct 10, 2024

LGTM label has been added.

Git tree hash: b90208103e9ab0bb9356f40d02326d8341d7f45a

FedeDP
FedeDP previously approved these changes Oct 10, 2024
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

@FedeDP
Copy link
Contributor

FedeDP commented Oct 10, 2024

Rebase & fix conflict? Then we will merge it :)

@LucaGuerra
new(falco): add buffer_format_base64
41cc537 
Signed-off-by: Luca Guerra <luca@guerra.sh>
@LucaGuerra
cleanup(falco): deprecate -b --print-base64
ec3b55d 
Signed-off-by: Luca Guerra <luca@guerra.sh>
@LucaGuerra LucaGuerra dismissed stale reviews from FedeDP and sgaist via ec3b55d October 10, 2024 15:01
@LucaGuerra LucaGuerra force-pushed the new/buffer_format_base64 branch from 5a00f2d to ec3b55d Compare October 10, 2024 15:01
@poiana poiana removed the lgtm label Oct 10, 2024
@poiana poiana requested review from FedeDP and sgaist October 10, 2024 15:01
FedeDP
FedeDP approved these changes Oct 10, 2024
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 Oct 10, 2024
@poiana
Copy link
Contributor

poiana commented Oct 10, 2024

LGTM label has been added.

Git tree hash: 8d339ea433bb9c4dbdaa831dc887a5f6a2a2f0c0

@poiana
Copy link
Contributor

poiana commented Oct 10, 2024

[APPROVALNOTIFIER] This PR is APPROVED

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

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:
  • OWNERS [Andreagit97,FedeDP,LucaGuerra]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

sgaist
sgaist approved these changes Oct 10, 2024
View reviewed changes
userspace/falco/app/actions/init_inspectors.cpp
Comment on lines +30 to +35
if(s.options.print_base64) {
falco_logger::log(falco_logger::level::WARNING,
"The -b/--print-base64 option is deprecated and will be removed. Use -o "
"buffer_format_base64=true instead.");
event_buffer_format = sinsp_evt::PF_BASE64;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Question for a followup: shouldn't that warning rather be printed when parsing the options ?

That way, you can keep all the deprecation related code in one single place.

@poiana poiana merged commit fb01b6d into falcosecurity:master Oct 10, 2024
34 checks passed
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

@sgaist sgaist sgaist approved these changes

@incertum incertum Awaiting requested review from incertum

@Kaizhe Kaizhe Awaiting requested review from Kaizhe

@Andreagit97 Andreagit97 Awaiting requested review from Andreagit97

Assignees

@sgaist sgaist

@FedeDP FedeDP

@Andreagit97 Andreagit97

Labels
approved area/engine dco-signoff: yes kind/feature lgtm release-note size/M
Projects
Falco Roadmap
Status: Done
Milestone
0.40.0
Development

Successfully merging this pull request may close these issues.
5 participants
@LucaGuerra @poiana @FedeDP @sgaist @Andreagit97