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

Refactor shelling out in Python #8167

Merged
merged 2 commits into from
Oct 12, 2023

Conversation

deivid-rodriguez
Copy link
Contributor

@deivid-rodriguez deivid-rodriguez commented Oct 10, 2023

There's a lot of duplicated code in the Python ecosystem that deals with shelling out to package managers. This PR refactors it to always use the same helper.

This fixes most of #6110.

}
)
def run_command(command, env: {}, fingerprint: nil)
SharedHelpers.run_shell_command(command, env: env, fingerprint: fingerprint)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a slight change where we will now raise stderr concatenated with stdout as the exception message, while before we'd only raise stdout.

raise SharedHelpers::HelperSubprocessFailed.new(
message: stderr_to_stdout ? stdout : "#{stderr}\n#{stdout}",
error_context: error_context
)

It seems like a good change to me.

@@ -204,7 +203,7 @@ def run_poetry_update_command
end

def run_poetry_command(command, fingerprint: nil)
Helpers.run_poetry_command(command, fingerprint: fingerprint)
SharedHelpers.run_shell_command(command, fingerprint: fingerprint)
Copy link
Contributor Author

@deivid-rodriguez deivid-rodriguez Oct 10, 2023

Choose a reason for hiding this comment

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

Same as the other comment, but reversed. This used to raise an error including only stderr, now we'll raise an error including stdout and stderr concatenated.

Seems fine too I guess!

@@ -309,7 +308,7 @@ def lockfile
end

def run_poetry_command(command, fingerprint: nil)
Helpers.run_poetry_command(command, fingerprint: fingerprint)
SharedHelpers.run_shell_command(command, fingerprint: fingerprint)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same as the other comment in the other run_poetry_command method.

@deivid-rodriguez deivid-rodriguez marked this pull request as ready for review October 10, 2023 16:06
@deivid-rodriguez deivid-rodriguez requested a review from a team as a code owner October 10, 2023 16:06
Copy link
Contributor

@yeikel yeikel left a comment

Choose a reason for hiding this comment

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

It's always great to see these cleanups and more so when the tests are happy 🥇

Copy link
Member

@Nishnha Nishnha left a comment

Choose a reason for hiding this comment

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

This LGTM. I would maybe get another approval from the team though since I haven't used time_taken in my debugging, but maybe someone else has found it useful and doesn't want it removed?

@deivid-rodriguez
Copy link
Contributor Author

deivid-rodriguez commented Oct 12, 2023

@Nishnha Note that this is not removing time_taken, the same code is present in the common run_shell_command helper which is now used consistently:

def self.run_shell_command(command,
allow_unsafe_shell_command: false,
env: {},
fingerprint: nil,
stderr_to_stdout: true)
start = Time.now
cmd = allow_unsafe_shell_command ? command : escape_command(command)
if stderr_to_stdout
stdout, process = Open3.capture2e(env || {}, cmd)
else
stdout, stderr, process = Open3.capture3(env || {}, cmd)
end
time_taken = Time.now - start
# Raise an error with the output from the shell session if the
# command returns a non-zero status
return stdout if process.success?
error_context = {
command: cmd,
fingerprint: fingerprint,
time_taken: time_taken,
process_exit_value: process.to_s
}
raise SharedHelpers::HelperSubprocessFailed.new(
message: stderr_to_stdout ? stdout : "#{stderr}\n#{stdout}",
error_context: error_context
)
end

Only changes in behavior should be the ones I noticed as inline comments in the PR.

@deivid-rodriguez deivid-rodriguez force-pushed the deivid-rodriguez/refactor-python-helpers branch from e73094e to c0d56c0 Compare October 12, 2023 16:58
@deivid-rodriguez deivid-rodriguez merged commit 7803593 into main Oct 12, 2023
80 checks passed
@deivid-rodriguez deivid-rodriguez deleted the deivid-rodriguez/refactor-python-helpers branch October 12, 2023 17:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants