Skip to content

Conversation

@ThomasDevoogdt
Copy link
Contributor

@ThomasDevoogdt ThomasDevoogdt commented Apr 22, 2025

Fixes:

Summary by CodeRabbit

  • New Features

    • SQLDB support is now optional; enabling it unlocks partial-match checklist and enhanced blob upload tracking.
    • Added a new post-upload action: "Add Suffix".
  • Bug Fixes

    • Non-database builds now clearly reject DB-only modes/events and fail fast when unsupported.
    • Improved init and cleanup paths to prevent leftover state on failure.
  • Tests

    • CI and tests run both DB-enabled and DB-disabled variants; DB-only tests are excluded when DB is off.

✏️ Tip: You can customize this high-level summary in your review settings.

@patrick-stephens
Copy link
Collaborator

Let's make sure this compiles for all existing targets as well.

Would it be better to disable the features within the plugins that need the DB rather than the whole plugin @leonardo-albertovich ? It feels a bit like a large hammer, e.g. no tail input even if you don't use db. We would have to make the config options and any other usage conditional though so it may be worse.

@ThomasDevoogdt
Copy link
Contributor Author

Let's make sure this compiles for all existing targets as well.

How can it not? Currently, FLB_SQLDB is enabled by default, and will stay like that. The problem is the other way around, if not enabled, then compilation breaks.

Would it be better to disable the features within the plugins that need the DB rather than the whole plugin @leonardo-albertovich ? It feels a bit like a large hammer, e.g. no tail input even if you don't use db. We would have to make the config options and any other usage conditional though so it may be worse.

For me fine, but I would do that on a per feature basis. Perhaps just continue with this PR, and then fine-tune some features that might compile with some small fixups.

In general, it would be much better if all options are toggled in the automated tests, on one reference compilation system. Perhaps even by incremental builds. But either way, I just want to fix compilation now.

@ThomasDevoogdt
Copy link
Contributor Author

ThomasDevoogdt commented May 2, 2025

@patrick-stephens @edsiper I changed this PR so that plugins are still compiled, but without database support. I hope this answers #10239 (comment).

Tested by doing this:

cd build/
cmake -GNinja -DFLB_SQLDB=OFF ../
ninja

and

cd build/
cmake -GNinja -DFLB_PREFER_SYSTEM_LIBS=ON -DFLB_SQLDB=OFF ../
ninja

@patrick-stephens
Copy link
Collaborator

Not sure if the CI failure is relevant or something else

@ThomasDevoogdt
Copy link
Contributor Author

Not sure if the CI failure is relevant or something else

I just added #ifdef FLB_HAVE_SQLDB, which is true by default, so I don't think it's relevant.

@cosmo0920
Copy link
Contributor

We fixed no left device error on our CI. So, could you rebase off current master?
We need to confirm all of the CI tasks are passed before processing review.

@ThomasDevoogdt
Copy link
Contributor Author

We fixed no left device error on our CI. So, could you rebase off current master? We need to confirm all of the CI tasks are passed before processing review.

Rebased, and all tests seems to be fine. Can we merge?

@patrick-stephens
Copy link
Collaborator

@leonardo-albertovich just checking your comments were covered?

@leonardo-albertovich
Copy link
Contributor

I'll review this PR again tomorrow. @ThomasDevoogdt if you have any questions that were unanswered (I think I missed some) please repeat them so I can address them tomorrow.

@ThomasDevoogdt
Copy link
Contributor Author

I'll review this PR again tomorrow. @ThomasDevoogdt if you have any questions that were unanswered (I think I missed some) please repeat them so I can address them tomorrow.

I don't have any open questions. If you see any shortcomings in this PR, then I will try to address them. But beware that I won't do any in depth refactorings, for that, someone closser to the matter should step in.

@ThomasDevoogdt
Copy link
Contributor Author

I'll review this PR again tomorrow. @ThomasDevoogdt if you have any questions that were unanswered (I think I missed some) please repeat them so I can address them tomorrow.

@leonardo-albertovich I think this PR is finalized, it would be nice having it merged. By this, the runtime tests would also start to work, limiting the chance that I have to rework this anytime soon.

@leonardo-albertovich
Copy link
Contributor

I don't have merge privileges anymore so you should ask @edsiper about it and wait for him to have time to dedicate to this.

@ThomasDevoogdt
Copy link
Contributor Author

@edsiper @patrick-stephens ping ...

@ThomasDevoogdt
Copy link
Contributor Author

@edsiper @patrick-stephens ping ...

Hi, this week is perhaps the ideal moment to merge it? @edsiper

@ThomasDevoogdt
Copy link
Contributor Author

@patrick-stephens Thx for the approval, can you also merge this?

@ThomasDevoogdt
Copy link
Contributor Author

@patrick-stephens @edsiper Happy New Year! 🎉

It's perhaps the ideal moment to merge this pull request now.

@ThomasDevoogdt
Copy link
Contributor Author

@patrick-stephens @edsiper Again a week later. Can this be merged please?

@ThomasDevoogdt
Copy link
Contributor Author

@patrick-stephens @edsiper ping

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ok-package-test Run PR packaging tests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants