Revert "clean up invoke output (#538)" #552
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This reverts commit ce8ab8a.
gviedma-zz
approved these changes
Mar 8, 2019
|
sorry, will try to add a test for this one, thanks for debugging. |
rdallman
added a commit
that referenced
this pull request
Mar 27, 2019
…fore creating the request, to appease the oci request signing code as it expects content length to be set and will not set it itself after reading the body. this fixes #552 diff of previous: clean_invoke...invoke_refix
Merged
rdallman
added a commit
that referenced
this pull request
Mar 28, 2019
* clean up invoke output (#538) * clean up invoke output * no more async output catch (not a thing, at the moment) * we can get fn errors that have a call id, so attempt to unwind them first instead of just outputting them in json format. this should still output user's function output even for non-200 responses the same, unless they use the same json we do for errors which are non-empty, in which case adding some window dressing from the cli doesn't seem horrible? * adds a \n to the output of the cli invoke to make terminals more happy with it below displays the issues we had previously where we were showing the full json error from fn instead of unraveling it to show to the user, also we were previously showing the help message if the function invoke returned >400, which could denote a successful invocation that simply returns a >400 (from a user perspective) - cleaned both of these up, and merged the errors into one line instead of 2. I'm open to feedback on formatting the error, I'm tempted to just do `Error invoking function: 504 Timed out` - open to other ideas. master: ``` ✗: cli invoke testapp hello {"message":"Timed out"} Fn: Error calling function: status 504 See 'fn <command> --help' for more information. Client version: 0.5.51 ``` now: ``` ✗: cli invoke testapp hello Error invoking function. status: 504 message: Timed out ``` success output is the same: ``` ✗: cli invoke testapp hello {"message":"Hello World"} ``` except now if the function doesn't output a \n last, then the cli will, this closes #1408 so that we play nice with the terminal * add early exit * invoke json output, further cleaning we can get headers out of cli invoke now, huzzah. this does a good amount of tidying up the client.Invoke function, which was pretty raw, and had some stale stuff on it even. mainly plumbs output out so that the cli can switch on its flags for output, instead of passing the output and flags down into the client invoke function. returning *http.Response always sucks, but at least in CLI program it will terminate quickly and the conns will be cleaned up, we handle closing the body fine in the program for now at least though. removed the block to read the request body into a buffer and limit its size, that seems like the wrong thing to do overall, it means the cli limits the input so that the server doesn't error about max size, but the input won't be complete so a function itself could error (imagine, a 1GB JSON that gets chopped off at 6MB or whatever) - we should throw this at the server until the server yells that it's too big, and the server should be limiting this (there's options that are supposed to be turned on). I had an idea to just output raw HTTP too, this is easy to add, bringing up for discussion here first though. looks like: ``` ✗: cli invoke --output=json testapp hello { "body": "{\"message\":\"Hello World\"}\n", "headers": { "Content-Length": [ "26" ], "Content-Type": [ "text/plain; charset=utf-8" ], "Date": [ "Thu, 28 Feb 2019 06:33:33 GMT" ], "Fn-Call-Id": [ "01D4SCXRK5180043RZJ0000005" ], "Fn-Http-Status": [ "200" ] }, "status_code": 200 } ``` * remove stale comment * test output not exit code I kinda liked that we exit with error but it's really confusing, it doesn't say anything about whether the request itself failed or not, and we get into specific error parsing. * this is the same as #538 except for a fix that copies all of stdin before creating the request, to appease the oci request signing code as it expects content length to be set and will not set it itself after reading the body. this fixes #552 diff of previous: clean_invoke...invoke_refix
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This reverts commit ce8ab8a. The changes in the commit caused invokes with a payload to fail with 401 Not Authenticated. Initial investigation seems to show the body might not be getting sent correctly. Without the offending commit,
echo -n '{name: foo}' | DEBUG=1 fn invoke harry-test helloshows{name: foo}as the body of the request in the debug output. With the changes in the commit, the same CLI command showsas the body in the debug output. Reverting the commit for now until the issue is fixed.