-
Notifications
You must be signed in to change notification settings - Fork 0
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
W-16451183 Wr/file flag #14
Conversation
file: Flags.file({ | ||
summary: messages.getMessage('flags.file.summary'), | ||
helpValue: 'file', | ||
char: 'f', |
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.
you can pass exists
to have oclif validate the file:
https://github.com/oclif/core/blob/f38f8270bafa04cdcb8e6d193b808e099e205864/src/flags.ts#L121
also, we haven't been consistent with this but since this command already has --stream-to-file
, what do you think about using --request-file
instead?
https://github.com/search?q=org%3Asalesforcecli+%2FFlags%5C.file%5C%28%2F&type=code
https://github.com/salesforcecli/cli/wiki/Design-Guidelines-Flags
src/commands/api/request/rest.ts
Outdated
const fileOptions = flags.file | ||
? (JSON.parse(fs.readFileSync(flags.file, 'utf8')) as { | ||
body?: string; | ||
header?: 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.
this should be headers
(flag is singular because we expect user to pass it multiple times)
src/commands/api/request/rest.ts
Outdated
: // otherwise it's a stdin, and we use it directly | ||
flags.body; | ||
} else { | ||
body = JSON.stringify(fileOptions.body); |
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.
cc @VivekMChawla @WillieRuemmele
Today --body
can be a body string, -
to read from stdin or a file path to read the body from.
Since this PR adds support for passing a request as a file I feel we should drop --body
from supporting file paths (it's the same feature but we are opening 2 ways to use it).
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.
@WillieRuemmele - I think that @cristiand391's idea to drop support for file paths in the --body
flag makes sense.
messages/rest.md
Outdated
# flags.method.summary | ||
|
||
HTTP method for the request. | ||
|
||
# flags.file.summary | ||
|
||
A json file to store values for header/body/method/url - this is the same format as a Postman Collection Format |
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.
@WillieRuemmele @VivekMChawla I couldn't find a way to export a postman request in the collection format.
If it's not possible then we might not need to follow their format?
context:
by not following their format we leave this open to us adding support for parameterized params later:
https://salesforce.quip.com/5t19A9rykzI3#SdNADAvRyDL
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.
looks good, left a few questions regarding functionality/formats (will ping Vivek) and get back.
QA notes: arg/flags override value in body file✅ endpoint as ARG others✅ HTTP method still defaults to body format✅ NOTE: it's missing the {
"url": "limits",
"header": [{
"value": "application/xml"
}]
}
✅ raw body mode, can define request body as JSON (gets stringified in payload) NOTE: it's missing the {
"url": "sobjects/Account/0018G00000bTTg4QAG",
"method": "PATCH",
"body" : {
"raw": {
"name": "Mr Doe"
}
}
}
we should either default to
|
QA update:
✅ fixed:
✅ fixed
|
What does this PR do?
adds
--file
flag torequest rest
commandmatches postman collection format e.g. keys are body/header/url/method
What issues does this PR fix or reference?
@W-16451183@