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

refactor(userspace): move falco logger under falco engine #3208

Merged
merged 1 commit into from
May 23, 2024

Conversation

jasondellaluce
Copy link
Contributor

What type of PR is this?

/kind bug

/kind cleanup

Any specific area of the project related to this PR?

/area engine

What this PR does / why we need it:

Since introducing the method evttype_index_ruleset::print_enabled_rules_falco_logger(), we created an implicit dependency between the falco_engine target and the falco logger, which instead lives under the falco_application target. The two targets don't depend on each other and the linking succeeds just because they're both included by the falco cmake executable target. Pedantic linker checks will have this failing instead, and is fundamentally wrong even if it works by luck.

There are many ways for overcome this, but my proposal is to just move the falco logger as part of the falco engine's code. There are many instances in which logging would be useful and it hasn't been done exactly because of this uncomfortable dependency, so moving the component seems like an easy and appropriate fix IMO.

Which issue(s) this PR fixes:

Special notes for your reviewer:

I'll let the reviewers decide on the right milestone. IMO this may be 0.38.0 material for better code quality.

Does this PR introduce a user-facing change?:

refactor(userspace): move falco logger under falco engine

Copy link

This PR may bring feature or behavior changes in the Falco engine and may require the engine version to be bumped.

Please double check userspace/engine/falco_engine_version.h file. See versioning for FALCO_ENGINE_VERSION.

/hold

@jasondellaluce
Copy link
Contributor Author

/unhold

False positive

@FedeDP
Copy link
Contributor

FedeDP commented May 21, 2024

Question, what about creating an entire new target falco_utils (or whatever) that will be then liked by both falco_application and falco_engine?
I mean, aren't we still creating a link between falco_engine and falco_application with your solution?
Also, i can see people struggling to find source code for the falco logger that imo does not seem to belong to the engine, at a first glance at least.

@jasondellaluce
Copy link
Contributor Author

I mean, aren't we still creating a link between falco_engine and falco_application with your solution?

No, this for now preserves the single non-circular falco_application -> falco_engine dependency and just focuses on moving the logger in a lower-level component in the dependency tree where it could be more needed. I agree there's a need of polishing the components and targets of the Falco codebase and we should tackle that too, however I'd like to scope this contribution to a simple fix rather than an incremental refactor of the project structure, which instead will take maybe good design decisions later on. WDYT?

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
Copy link
Contributor

poiana commented May 23, 2024

LGTM label has been added.

Git tree hash: e6f05b0208f6c6cc688d175b94cb527e47d26ceb

@FedeDP
Copy link
Contributor

FedeDP commented May 23, 2024

Yep let's go with this!
/milestone 0.38.0

@poiana poiana added this to the 0.38.0 milestone May 23, 2024
@poiana
Copy link
Contributor

poiana commented May 23, 2024

[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:
  • OWNERS [FedeDP,jasondellaluce,leogr]

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 cd1c5f9 into master May 23, 2024
35 checks passed
@poiana poiana deleted the fix/logger-engine-link-issue branch May 23, 2024 07:29
@FedeDP FedeDP mentioned this pull request May 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants