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

Stream output from launched command #40

Merged
merged 2 commits into from
Feb 17, 2024

Conversation

afh
Copy link
Contributor

@afh afh commented Feb 17, 2024

Envio collects the output of launched commands, if the command is short-lived that is reasonable, yet if the command takes a long time and streams messages to stdout and/or stderr seeing these messages as the command runs is very helpful.

Image a simple script that prints the current date and time every second:

#!/bin/sh

while :; do
  date
  sleep 1
done

When launched via envio profile -c 'sh ./clock.sh nothing is printed to the terminal. With the changes proposed in this PR however the date and time appears every second.

What are your thoughts on this, @humblepenguinn?

Copy link
Collaborator

@humblepenguinn humblepenguinn left a comment

Choose a reason for hiding this comment

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

Using unwrap isn't a good idea. It would be better if we properly handled the error before continuing.

I seem to have made the same mistake when using String::from_utf8(output.stdout).unwrap()

@humblepenguinn
Copy link
Collaborator

But the idea is really good. Seeing the command output is very crucial, without that whats the point of the launch command.

Thank you @afh!

@afh
Copy link
Contributor Author

afh commented Feb 17, 2024

Thank you for pointing out that unwrap should be avoided, I'm not familiar enough with Rust to know best practices. What is your advice on how to improve the code?
I'm thinking if-let-Ok, e.g.:

                if let Ok(status) = cmd.wait() {
                    if let Some(code) = status.code() {
                        std::process::exit(code);
                    }
                }

@humblepenguinn
Copy link
Collaborator

humblepenguinn commented Feb 17, 2024

I believe something like the following would work:

let status = match cmd.wait() {
        Ok(s) => s,
        Err(e) => {
            println!("{}: {}", "Error".red(), e); 
            exit(1);
        }
    };
    
match status.code() {
        Some(code) => std::process::exit(code),
        None => {
            println!("{}: Child process terminated by signal", "Error".red());
            std::process::exit(1);
        }
}

I want to be able to also print out the error message. if let does work however in that case if an error occurs we won't know about it, which isn't quite what we want. So we use a match statement instead.

I made the changes required and merged the pull request. Thank you @afh

@humblepenguinn humblepenguinn merged commit ea49184 into envio-cli:main Feb 17, 2024
@afh afh deleted the stream-launch-output branch February 17, 2024 15:20
@afh
Copy link
Contributor Author

afh commented Feb 17, 2024

Thank you for improving this initial PR then reviewing, and merging it, very much appreciated! 😃

@humblepenguinn
Copy link
Collaborator

I wanted to ask you if you would be interested in being a maintainer for the project?

@afh
Copy link
Contributor Author

afh commented Feb 19, 2024

Thank you for asking, @humblepenguinn, and apologies for the late response. I allowed myself an extra day to consider your request and think it through.

At this point I kindly decline to be a maintainer for this project for two reasons:
a) Given other obligations I have and other interests I pursue I do not want to take on additional long-term commitments
b) I feel my knowledge of Rust today is too limited in order to be helpful as a maintainer

I'll happily continue to contribute to this project as long as the ideas flow and you find them helpful.

Maybe in the future I've accumulated more knowledge about Rust and other interests have subsided freeing up time and brain cycles for me to spend on this project.

@humblepenguinn
Copy link
Collaborator

No worries @afh, I completely understand your points and your current position.

Your contributions thus far have been immensely valuable, and I appreciate your honesty about your current commitments and knowledge level. Your future ideas and contributions will be eagerly welcomed!

And who knows, perhaps in the future, circumstances will change, and we can revisit the possibility of you taking on a more formal role once again 🌟

Thank you again for all that you have done for the project!

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.

2 participants