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

Exit early on patch failure if bloom is being run non-interactively. #668

Merged
merged 5 commits into from
Apr 19, 2022

Conversation

nuclearsandwich
Copy link
Contributor

If git-bloom-release is run non-interactively then that should be
propagated to the actions command so that actions are not blocked on
interactivity.

This patch uses an environment variable to send that message since the action commands do not have a --non-interactive flag and their command-line arguments are all defined statically in the tracks file so using the environment makes more sense than further munging those tracks unless we consider adding a {non-interactive} template to the tracks string, which would require updating all outstanding tracks files, also not my favorite idea.

I haven't audited the code for additional interactive points inside tracks files and I'm not yet sure if I mean to or if I plan to play "whack-a-mole" with them as I find them naturally over time.

If git-bloom-release is run non-interactively then that should be
propagated to the actions command so that actions are not blocked on
interactivity.

This patch also makes the minor optimization of only making one copy of
the environment to pass to the actions run. I don't think this makes a
major difference in performance but it is one less thing to worry about
and environment changes from children should not persist to subsequent
children or "bubble up" to the parent process.
@cottsay
Copy link
Member

cottsay commented Apr 18, 2022

This patch uses an environment variable to send that message since the action commands do not have a --non-interactive flag and their command-line arguments are all defined statically in the tracks file so using the environment makes more sense than further munging those tracks unless we consider adding a {non-interactive} template to the tracks string, which would require updating all outstanding tracks files, also not my favorite idea.

This is tricky. Some action list entries take -y, but not all. We're in a strange state with non-interactive as a whole, though, because the existing default action list is passing -y but we're not respecting that during patching, obviously. So -y has a different meaning when passed to an action than it does when passed to git-bloom-release at the top, where it seems to mean "I'm running in a script so don't ask for input".

Maybe a better approach to running in a script would be to just close stdin and make sure the prompts can handle it. I don't know. It sounds like a non-zero amount of work.

tl;dr - this change doesn't make the madness all that much worse, but I'm still not thrilled about it. If it's needed to unblock progress, then I think it's OK to merge.

@nuclearsandwich
Copy link
Contributor Author

Maybe a better approach to running in a script would be to just close stdin and make sure the prompts can handle it. I don't know. It sounds like a non-zero amount of work.

I gave this a shot by setting stdin=subprocess.DEVNULL in the subprocess call for git bloom-release was enough to break the interactivity but it also causes bloom to assume the patch imports successfully so I think some work is going to be required still to make it "fail" on no input.

This reverts commit a9fe30c.
The updated implementation relies on using isatty for interactive
terminal detection rather than an exported environment variable.
Copy link
Member

@cottsay cottsay left a comment

Choose a reason for hiding this comment

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

I like this SO much better. We should change the rosdep rule failure to do the same thing. That way, -y only suppresses non-failure interactivity. To automatically fail the command instantly, you close stdin.

Another time perhaps.

@nuclearsandwich nuclearsandwich changed the title Exit early on patch failure if bloom is being run non-interactively. [migration-tools] Exit early on patch failure if bloom is being run non-interactively. Apr 19, 2022
@nuclearsandwich nuclearsandwich changed the title [migration-tools] Exit early on patch failure if bloom is being run non-interactively. Exit early on patch failure if bloom is being run non-interactively. Apr 19, 2022
@cottsay cottsay mentioned this pull request Apr 19, 2022
@nuclearsandwich nuclearsandwich merged commit 512ecbe into master Apr 19, 2022
@nuclearsandwich nuclearsandwich deleted the noninteractive-bail-on-am-fail branch April 28, 2024 15:20
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.

3 participants