-
Notifications
You must be signed in to change notification settings - Fork 31
Introduce process API #418
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
Conversation
411b40e
to
3b231df
Compare
* | ||
* @param <O> the output type | ||
*/ | ||
public sealed interface ProcessBuilder<O> |
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.
is reusing ProcessBuilder name that overlaps with jdk ProcessBuilder a good thing?
asking as I hit that IDE default imported java.lang.ProcessBuilder when pasting code :)
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.
Maybe. I don't know. I thought that any code using this would definitely not be using the JDK one. The fact that the JDK one is in java.lang
is a bit annoying though. But I couldn't actually think of any better name... 🤔
/** | ||
* {@return the captured error output of the process execution, if any} | ||
*/ | ||
public List<String> errorOutput() { |
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.
is this stderr only ? what about stdout?
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.
We could capture stdout here, but doing so would mean adding "tee" support broadly. This can be done, but would require more effort. It's also possible that we could introduce this in the future.
process/src/main/java/io/smallrye/common/process/ProcessExecutionException.java
Outdated
Show resolved
Hide resolved
Not implemented, and not sure about relative importance:
|
1535999
to
dfb8585
Compare
I'm going to provisionally merge this so that I can push up a beta release, which will allow for comprehensive testing and review in Quarkus. We should not release a final version until we have a broad consensus that the API is OK. |
As promised in quarkusio/quarkus#48223, here's the draft Process API.
The tests (so far) pass. I want to add more tests and try replacing some usages in Quarkus itself before I will call this ready to merge, but feel free to review now.