Skip to content

Load "all" plugins by default & allow "empty" values #2689

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

Merged
merged 36 commits into from
Nov 18, 2022

Conversation

Dakostu
Copy link

@Dakostu Dakostu commented Nov 8, 2022

Load "all" plugins if no "plugins" configuration key is defined in the configuration.

📝 Reviewer Checklist

Review this pull request by ensuring the following items:

  • All user-facing changes have changelog entries
  • User-facing changes are reflected on vast.io

@Dakostu Dakostu requested a review from a team November 8, 2022 14:24
@Dakostu Dakostu added the feature New functionality label Nov 8, 2022
Copy link
Member

@dominiklohmann dominiklohmann left a comment

Choose a reason for hiding this comment

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

Just one minor comment. Thanks for tackling this one; I'll leave the remaining review to the team.

@mavam
Copy link
Member

mavam commented Nov 9, 2022

This is makes the OOTB experience so much better!

@Dakostu Dakostu marked this pull request as ready for review November 9, 2022 11:55
@Dakostu Dakostu force-pushed the story/sc-38995 branch 2 times, most recently from 599148f to 8850239 Compare November 11, 2022 10:01
@Dakostu Dakostu changed the title Load "all" plugins by default Load "all" plugins by default & allow "empty" values Nov 11, 2022
@dominiklohmann
Copy link
Member

@tobim can I ask you to take the review from here on out?

Copy link
Member

@tobim tobim left a comment

Choose a reason for hiding this comment

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

I tested this change locally and all my calls to vast start and other sub commands behaved as expected.
Ready to be merged once the code change request is addressed.

Copy link
Member

@dominiklohmann dominiklohmann left a comment

Choose a reason for hiding this comment

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

This is great, just some smaller things left to do. I also added a blocked label to prevent this from being merged accidentally in the feature freeze for v2.4; whether this gets in regardless is up to @lava.

@dominiklohmann dominiklohmann added the blocked Blocked by an (external) issue label Nov 16, 2022
@dominiklohmann
Copy link
Member

Since the first rc will be tagged rc2 anyways we decided to squeeze this in in today's standup.

@dominiklohmann dominiklohmann removed the blocked Blocked by an (external) issue label Nov 18, 2022
dominiklohmann added a commit that referenced this pull request Nov 18, 2022
This is a code base cleanup related to #2689.

Historically speaking, "native plugins" were functionality that was part
of libvast but used the plugin API. This allowed us to have defaults
that we ship as built-in functionality. The only reason why we did not
name them builtins from the get-go was because the term builtin was too
close to the term bundled, which we already used for plugins that were
built alongside VAST in the same build configuration (via
`-DVAST_PLUGINS=...`). Now that bundled plugins are a purely internal
term we can instead start referring to native plugins as builtins.

In addition, this changes the show command to be a built-in instead
without making any functional changes to it.
@dominiklohmann dominiklohmann merged commit a31abb9 into master Nov 18, 2022
@dominiklohmann dominiklohmann deleted the story/sc-38995 branch November 18, 2022 11:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants