Fixes #1595: Add method to process.py to return both return code and logs

[favilo]

As requested in #1595

[x] Have you signed the contributor license agreement?
[x] Have you followed the contributor guidelines?
[x] Have you run make check-all successfully?
[x] Did you choose a descriptive title and description for your PR?

• (Only for maintainers) Did you apply appropriate labels and a milestone?

[gbanasiak]

LGTM, but #1595 lacks explicit clarification of what's exactly the win. I think we can find it here.

WDYT about using the modified run_subprocess() to avoid the double call in is_branch() as part of this PR? On quick scan that's probably the only improvement in the git module we can make as other methods use different commands in a sequence?

[favilo]

LGTM, but #1595 lacks explicit clarification of what's exactly the win. I think we can find it here.

WDYT about using the modified run_subprocess() to avoid the double call in is_branch() as part of this PR? On quick scan that's probably the only improvement in the git module we can make as other methods use different commands in a sequence?

What do you think about the latest change?

[gbanasiak]

What do you think about the latest change?

Thank you for iterating. 21caafa eliminates duplication but made me realize we're missing the logging now which I think should be kept for troubleshooting purposes. We could add snowflake logging at git module level, or add logging option to run_subprocess(). Or, rewrite run_subprocess_with_logging() to be based on subprocess.run() and return CompletedProcess, but this would extend the scope of the PR. Which option do you find the most elegant?

[favilo]

What do you think about the latest change?

Thank you for iterating. 21caafa eliminates duplication but made me realize we're missing the logging now which I think should be kept for troubleshooting purposes. We could add snowflake logging at git module level, or add logging option to run_subprocess(). Or, rewrite run_subprocess_with_logging() to be based on subprocess.run() and return CompletedProcess, but this would extend the scope of the PR. Which option do you find the most elegant?

Adding logging option to run_subprocess() feels like the least effort solution here. I wouldn't mind having run_subprocess_with_logging() return a Popen object which would also have .returncode and .stdout, but that has a lot more uses, and since we are already logging, do we need to keep track of the output separately?

I could certainly be convinced that the latter one is the correct approach, but maybe not in this small PR?

EDIT: But then again, the original usecase that brought about this change was needing to do exactly that, so I can run through and add a .returncode to all of those uses, and it will make things more obvious what is going on. Of course, adding type signatures here would also help in the long run.

[gbanasiak]

IT tests are failing as it turns out run_subprocess_with_logging() is also used in https://github.com/elastic/rally-teams, e.g. here. I wasn't aware of that. This changes the picture considerably. I think we should go for a new method in the process module for backward compatibility. The problem is https://github.com/elastic/rally-teams code can be interpreted by a range of Rally versions.

[gbanasiak]

LGTM. Please change the title of the PR as it's no longer relevant. I've left 2 minor comments.

[gbanasiak]

Suggested change

	Runs the provided command line in a subprocess. All output will be returned in the `CompletedProcess.stdout` field.
	Runs the provided command line in a subprocess.

[gbanasiak]

Nit: Should type definitions be moved to the top of the file?

[favilo]

Huh, I see the IT tests failed, but looking at the logs, I don't actually see any failures noted.