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
Remove the use of shell for mkdir -p #2314
Conversation
Release 0.50.1
Release 0.50.2
See https://stackoverflow.com/questions/600268/mkdir-p-functionality-in-python Signed-off-by: Michael Scherer <misc@redhat.com>
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.
Just missing some exception handling.
|
||
raise Exception("Could not create git repo's prerequisite directories. " | ||
" Do you have write access?") | ||
pathlib.Path(repo_path).mkdir(parents=True, exist_ok=True) |
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.
This is a good change however it should still catch any exceptions Path.mkdir raises and respond like how it did before when we didn't get a return code. It should update its internal logs when it is unable to create the directory.
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.
Just put the mkdir line in a try/except block and put the body of the if statement on line 123 in the exception handle. Otherwise looks good.
handle when mkdir call doesn't work for whatever reason.
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.
Looks good to me now.
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.
@IsaacMilarky / @ABrain7710 : Could you go ahead and review this?
See https://stackoverflow.com/questions/600268/mkdir-p-functionality-in-python
Description
This PR fixes a problem I bumped into while deploying (eg, the container do not work without /bin in PATH). I have a workaround for my setup, but it is better to fix the root cause.
Notes for Reviewers
Signed commits