-
Notifications
You must be signed in to change notification settings - Fork 33
Cext 5368/minimal db provisioning #889
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
Cext 5368/minimal db provisioning #889
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
nofuss
left a comment
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.
Some questions and considerations.
src/commands/app/deploy.js
Outdated
| spinner.start(message) | ||
| } | ||
|
|
||
| await this.config.runCommand('app:db:provision', ['--region', region, '--yes']) |
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.
For the moment running db provision in a workspace where a database has already been provisioned (it does nothing), but that might change in the future.
It would be safer to run db status first. If a database is already provisioned nothing more need be done. Also, if a database is already provisioned but in a different region than what is in the manifest, that should be treated at least as a warning.
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.
yeah, initially i was doing that but the reason i skipped that the status command check first is that provision command already does status check before provision and logs the details, so running status before provisioning felt kind of repetitive here
|
Since the |
Yes, generally the way we have implemented this is that the app command does not call the other command but instead uses the lib. We also need to keep in mind that this release is probably going to come after an alpha|beta of aio-cli-plugin-storage with db commands, so this should not be merged until we are ready to release everything. |
|
@purplecabbage - I don't understand why you've labelled this a "breaking change". This PR does introduce a new feature, but I am not aware that it is backwards incompatible with the existing feature. Can you explain? |
|
Description
Allow a database to be declared in the manifest of an App Builder application and for that database to be automatically provisioned in the specified region when the application is deployed to a specific Project Workspace.
Updated deploy command to provision database if configured
Related Issue
https://jira.corp.adobe.com/browse/CEXT-5368
Related PR- adobe/aio-cli-lib-app-config#43
Motivation and Context
How Has This Been Tested?
linking package locally
Screenshots (if appropriate):
Types of changes
Checklist: