-
Notifications
You must be signed in to change notification settings - Fork 1
CEXT-5230: Releasing aio app db cli commands
#27
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
Co-authored-by: Peter Dohogne <pdohogne@adobe.com>
Feat/cext 4859
feat:cext-4860 added standalone commands
|
Part of the reason the (On the other hand, the syntax of So I'd be game for simplifying the db plugin syntax to reduce the clutter. Applying all your suggestions would end up with something like this: db: collections ( documents ( indexes: ( My only concern is that it is completely different from the @pdohogne-magento and @moritzraho what do you think?
|
|
@nofuss thanks for the review, I agree. a few points:
|
agreed - collection can get tiring to type
I think this would be a stretch. It would be comparable to having a "current table" in SQL, and that is hard to imagine. I honestly think adding the idea of a "current collection" would be more confusing than convenient.
yes. I included |
sounds good 🤝 |
|
My take on defaults for one vs many document operations:
Make sense for everybody? |
makes sense, so you would control many insert/deletes/.. through a flag (--many) + multiple args? |
The --many flag is more about how many documents are affected. Both updateOne and updateMany, for example, have basically the same args, but updateMany will update however many documents are matched by the filter, but updateOne will update one and only one document, even if the filter matches multiple documents. As for insert, we could make that simpler. If one document is provided, use insertOne, if a list of documents is provided, insertMany (no flag needed). |
|
Luckily combining |
moritzraho
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.
cleaan!!
lgtm :)
just one small comment
| ] | ||
|
|
||
| DeleteOne.args = { | ||
| Delete.args = { |
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.
should this take a many flag?
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.
At Manik's request we are not including deleteMany functionality in the CLI
AjazSumaiya
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.
nice work @pdohogne-magento , looks good
add --yes flag for provision command
package.json
Outdated
| { | ||
| "name": "@adobe/aio-cli-plugin-app-storage", | ||
| "version": "1.1.0", | ||
| "version": "1.2.0-alpha.1", |
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.
The version should be modified by an explicit release job. We will need to manage the pre version, and the dependency in a branch until we go GA
Description
Committing
aio app dbcommands to mainRelated Issue
CEXT-5230: Deploy CLI DB App to Staging
Motivation and Context
Release
aio app dbcommandsHow Has This Been Tested?
Unit and manual tests, see PRs #12 - #26
Screenshots (if appropriate):
Types of changes
Checklist: