-
Notifications
You must be signed in to change notification settings - Fork 662
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
cli-test(feat): Added datastore
CLI commands
#2041
Conversation
|
||
export interface DatastoreCommandArguments { | ||
/** @description datastore get <query> */ | ||
getQuery: string; |
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.
Here I'm using general string for datastore commands to make it easier than pre-define attributes such as datastore
, item
, and expression
, etc
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.
I feel like this will make it easy to make mistakes. I think we should model these just like we model the actual methods:
apps.datastore.get
method needsdatastore
string andid
stringapps.datastore.query
method needsdatastore
string, and its optional arguments, etc.
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.
It makes sense, I'd say for get
method, id
is not a must as it's defined in the manifest as primary key. I'd like to change by adding the datastore
as the new argument here.
Edit: my bad the id is always presented for get and delete command.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2041 +/- ##
==========================================
- Coverage 92.51% 91.74% -0.77%
==========================================
Files 37 38 +1
Lines 9981 10081 +100
Branches 631 631
==========================================
+ Hits 9234 9249 +15
- Misses 747 820 +73
- Partials 0 12 +12
Flags with carried forward coverage won't be shown. Click here to find out more. |
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.
I think we should model the API arguments as they are documented on our public docs 😞 sorry, I know that's annoying, but if we accept these JSON-payloads-as-strings, it would require a breaking change / major new version to change it to make it more accurate. I think the chance of making a typo with the string-based arguments is too high.
|
||
export interface DatastoreCommandArguments { | ||
/** @description datastore get <query> */ | ||
getQuery: string; |
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.
I feel like this will make it easy to make mistakes. I think we should model these just like we model the actual methods:
apps.datastore.get
method needsdatastore
string andid
stringapps.datastore.query
method needsdatastore
string, and its optional arguments, etc.
Summary
This PR adds basic
datastore
CLI commands for e2e test, it includesdatastore get/put/query/delete
Requirements (place an
x
in each[ ]
)