Split Admin CLI into modules per technology#3947
Split Admin CLI into modules per technology#3947dimas-b wants to merge 4 commits intoapache:mainfrom
Conversation
dimas-b
force-pushed
the
admin-refactor
branch
2 times, most recently
from
93accb2 to
c0d59ce
Compare
dimas-b
requested review from
clambertus and
flyrain
and removed request for
clambertus
| * Quarkus will find CLI commands only if they are `@Default`, so `@ApplicationScoped` cannot be | ||
| * applied to them... hence the workaround with this placeholder just to hold the | ||
| * `@ApplicationScoped` annotation. | ||
| */ |
There was a problem hiding this comment.
I understand the workaround. I just wonder if it's actually required ?
There was a problem hiding this comment.
Yes. Quarkus does not "see" custom qualifiers (like @AdminSubCommand). Since persistence/nosql/admin/cmd does not have any standard "bean-defining" annotations, Quarkus will not consider it when looking for @AdminSubCommand implementations.
I guess this is an interplay of some not-so-standard Quarkus approach to implementing CLI (note that Quarkus considered only @Default beans for CLI commands) and me wanting to leverage @AdminSubCommand to allow pluggable CLI modules.
| public abstract class BaseNoSqlCommand extends BaseCommand { | ||
| @Inject protected Backend backend; | ||
|
|
||
| public BaseNoSqlCommand() { |
There was a problem hiding this comment.
nit: Do we really need this constructor ?
There was a problem hiding this comment.
thx - leftover... removing
| @AdminSubCommand | ||
| @Default | ||
| public class NoSqlCommand extends BaseNoSqlCommand { | ||
| @Inject protected Backend backend; |
There was a problem hiding this comment.
backend is already defined in BaseNoSqlCommand. Do we really need to override it there ?
There was a problem hiding this comment.
Probably not... It was there before, I just did not notice 😅 Thx for the pointer - removing.
* Introduce `polairs-admin-base` for shared commands * Move NoSQL commands to `polaris-persistence-nosql-admin` * Make NoSQL commands optional with build property: `ExcludeNoSqlPersistence` as proposed in apache#3867 * Move JDBC-specific dependencies for admin commands to `polaris-relational-jdbc-admin` * CDI-based Sub-command discovery is handled by `AdminCliConfiguration` * Add dedicated admin test modules per persistence technology (JDBC / NoSQL) * Move default quarkus properties to `runtime-admin-defaults` to allow downstream projects to _not_ include them. * Downstream builds are advised to depend on individual command modules (noted above) and have local `application.properties`, but not depend on `runtime-admin`, which serves as the Admin CLI distribution assembly module for Apache Polaris releases. Closes apache#3867 This change also addresses most of concerns from apache#3855.
|
Note: I rebased to include new tests from #3352 |
2 participants
Introduce
polairs-admin-basefor shared commandsMove NoSQL commands to
polaris-persistence-nosql-adminMake NoSQL commands optional with build property:
ExcludeNoSqlPersistenceas proposed in Add build-time flags to produce Admin CLI without NoSQL dependencies #3867Move JDBC-specific dependencies for admin commands to
polaris-relational-jdbc-adminCDI-based Sub-command discovery is handled by
AdminCliConfigurationAdd dedicated admin test modules per persistence technology (JDBC / NoSQL)
Move default quarkus properties to
runtime-admin-defaultsto allow downstream projects to not include them.Downstream builds are advised to depend on individual command modules (noted above) and have local
application.properties, but not depend onruntime-admin, which serves as the Admin CLI distribution assembly module for Apache Polaris releases.Closes #3867
This change also addresses most of concerns from #3855.
Checklist
CHANGELOG.md(if needed)site/content/in-dev/unreleased(if needed)