Skip to content
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

(maint) Update task blocking response to return all results #639

Merged
merged 1 commit into from
Aug 30, 2017

Conversation

MikaelSmith
Copy link
Contributor

@MikaelSmith MikaelSmith commented Aug 29, 2017

In the task response, return exitcode and stderr as well as stdout. This
allows a consumer to skip querying status, as all the data they need for
a task is in the response.

The status query used to retrieve task results returns stdout, stderr,
and exitcode. This makes the response to running a task - which is
returned immediately upon completion, rather than in response to a
status request - contain the same information so a controller can avoid
a follow-up request to get it.

In the task response, return exitcode and stderr as well as stdout. This
allows a consumer to skip querying status, as all the data they need for
a task is in the response.

The status query used to retrieve task results returns stdout, stderr,
and exitcode. This makes the response to running a task - which is
returned immediately upon completion, rather than in response to a
status request - contain the same information so a controller can avoid
a follow-up request to get it.
@puppetcla
Copy link

CLA signed by all contributors.

@MikaelSmith
Copy link
Contributor Author

MikaelSmith commented Aug 29, 2017

2017-08-29 14:03:29,002 TRACE [qtp1067320520-19] [p.p.b.shared] Sending PCP message to pcp://puppet:8143/server: {:id "c8b1c15d-0301-44dd-8b65-d86b483d74f6", :message_type "http://puppetlabs.com/rpc_non_blocking_response", :target "pcp://puppet:8143/server", :sender "pcp://10-32-167-127.rfc1918.puppetlabs.net/agent", :data {:transaction_id "202", :results {:exitcode 0, :stdout "{"message":"hello"}\n"}}}

@MikaelSmith
Copy link
Contributor Author

@nicklewis did you have any more thoughts on this?

@nicklewis
Copy link
Contributor

No, I think it makes sense to me.

@MikaelSmith MikaelSmith merged commit be8d92e into puppetlabs:master Aug 30, 2017
@MikaelSmith MikaelSmith deleted the use_output branch August 30, 2017 21:36
result.set("stdout", output);
}
if (!response.output.std_err.empty()) {
result.set("stderr", response.output.std_err);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we sanitize this to ensure it is valid UTF-8?

Copy link
Contributor Author

@MikaelSmith MikaelSmith Aug 30, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would also want to do that in the else clause. Maybe, but I'm tempted to say it's unlikely we'll get binary data on stderr.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair enough.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants