-
Notifications
You must be signed in to change notification settings - Fork 443
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
Fixes for RPM related functionality in 1.0.5-M2 #668
Conversation
…clared Use normal bash `if` statement as opposed to `[ -n "${MY_VAR}" ] && ...` as the latter will cause the preinst script to fail if the condition check returns false
Manual Tests Scenario:
For each scenario, ensure:
Tested on Centos7 I think the issues raised in this PR is mainly caused by the fact that I was overriding the I have now taken a different approach:
With this approach, I now have a better confidence around the changes I have made. Hopefully this turns out to be the case. |
if [ -n "$RPM_INSTALL_PREFIX" ] ; | ||
then | ||
echo "PACKAGE_PREFIX=${RPM_INSTALL_PREFIX}" >> /etc/sysconfig/${{app_name}} | ||
fi |
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.
Were these changes necessary?
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.
Yes, without these changes the preinst
script will fail and abort should the if
condition turns out to be false.
Without rpmPrefix
being specified, the [ -n "$RPM_INSTALL_PREFIX" ]
will return false, and the preinst
will abort with a failure.
One small comment, otherwise LGTM |
Fixes for RPM related functionality in 1.0.5-M2
LGTM. Thanks :) |
Release in |
Thanks! |
The fixes address the following issues.
Ensure RPM preinst script continues normally when
rpmPrefix
is not declaredWithout this fix, when
rpmPrefix
is not declared, thepreinst
will terminate in error.Ensure application is running on the installed directory in the RPM-based distribution
This was fixed as part of PR #653 which was accidentally removed in PR #661. Tests are added to prevent this regression from occurring.