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

Set the path to python #90

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

dmonllao
Copy link

No description provided.

@@ -82,4 +82,6 @@
define('TEST_ENROL_LDAP_DOMAIN', 'ou=Users,dc=openstack,dc=org');
}

$CFG->pathtopython = 'python';
Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @dmonllao,
should be /usr/bin/python be more appropriate?

HTH,
Matteo

@dmonllao
Copy link
Author

Thanks for commenting Matteo. I've applied the suggested change.

To be honest, I've always seen paths to binaries specified using the full path, but I have never understood what is the advantage of it over just using the file name. I do understand that the difference when the script using just the bin file name does not know the system where it will be executed, but in this case we do.

@scara
Copy link
Contributor

scara commented Dec 10, 2018

TNX @dmonllao!
Using the absolute path is cleaner (IMHO), safer - the main reason, even if running in a container where layers are known - and quicker; otherwise, the system will look up in what defined in an environment variable called PATH following its ordering, being the env defined by that user context.

HTH,
Matteo

@danpoltawski
Copy link
Contributor

👍 to full path

@dmonllao
Copy link
Author

👍 thanks for the explanation Matteo. The patch was already updated using the full path so no further changes are required.

@dmonllao
Copy link
Author

dmonllao commented Jan 30, 2019

Any chance that we could merge this? The patch does not seem problematic.

@dmonllao
Copy link
Author

The failures reported by travis are unrelated to this patch.

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