Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Is it really
/var/usrlocal/bin
? Not/var/usr/local/bin
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.
@ingvagabund it is:
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.
should
{{ openshift.common.admin_binary }}
be used instead ofoadm
in the command? It has the absolute path for the oadm executableThere 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.
It is set to
/usr/local/bin/oadm
if the deployment is containerized. Based ongrep -rn admin_binary
I was not able to find any assignment for the AH. Plus, the fact does not cover/var/usrlocal/bin
case.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.
Perhaps, create a separate PR that extends the
openshift_facts
module and sets theopenshift.common.admin_binary
to the proper location. @ashcrow when does the/var/usrlocal/bin
applies and when/usr/local/bin
? Is there any rule to that? Optionally, the module could check if/var/usrlocal/bin/oadm
or/usr/local/bin/oadm
exists and then set theadmin_binary
variable properly.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.
/var/usrlocal/bin
should not really be used on Atomic. It is a detail of how/usr/local
is made writeable. Checked on the last version of RHELAH: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.
I don't think it will hurt anything having
/var/usrlocal/bin
in the path. Normally I'd go ahead and remove it since it should be the same as/usr/local/bin
but I'd rather not re-ci for it.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.
sure that is not a blocker, we can go forward and merge this. My observation was about setting
$PATH
instead of using{{ openshift.common.admin_binary }}
which should already have the correct path tooadm
.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.
Oh I see what you mean. I did do a quick dependency check and the role does not depend on
openshift_facts
however the playbook does.@ingvagabund would it be safe to use
openshift.common.admin_binary
in the role?