-
Notifications
You must be signed in to change notification settings - Fork 142
feat: use plugin manifest #4469
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
base: main
Are you sure you want to change the base?
Conversation
93e4484 to
06f41ee
Compare
|
Commit: 1677df6
32 interesting tests: 14 flaky, 7 KNOWN, 5 SKIP, 5 RECOVERED, 1 FAIL
Top 50 slowest tests (at least 2 minutes):
|
06f41ee to
11fb319
Compare
| # With database resource (both flags required together) | ||
| databricks apps init --name my-app --features=analytics \ | ||
| --warehouse-id=abc123 --database-instance=myinst --database-name=mydb | ||
|
|
||
| # With secret resource (both flags required together) | ||
| databricks apps init --name my-app --features=analytics \ | ||
| --warehouse-id=abc123 --secret-scope=myscope --secret-key=mykey | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So that means we cannot pass two secrets, is that correct?
IMO we should support such case - I can imagine that two plugins require separate secrets for them 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be honest I'm not a fan of defining the flags statically, if they are about providing dynamic arguments for a given template. And probably it is also limiting passing second secret (like in the comment before)
I'd suggest going with a dynamic approach - what do you think about doing what Helm (https://helm.sh/docs/helm/helm_install/) does: --set foo=bar? In that way we could provide multiple secrets, SQL warehouses, etc.
Even better, we could do --set analytics.warehouseId=foo - to avoid naming conflicts between plugins. Like:
--set analytics.lakebaseEndpoint=... --set myOwnPlugin.lakebaseEndpoint=...| } | ||
|
|
||
| // Remove analytics-specific files if analytics is not selected | ||
| if !selectedSet["analytics"] { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This shouldn't be hardcoded anymore, right?
(we spoke about getting rid of hardcoded server, but this one should be also avoided; of course it doesn't need to be fixed on this PR 👍)
Changes
Why
Tests