-
Notifications
You must be signed in to change notification settings - Fork 2
Add SLES as a valid OS for detect_os #85
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
Conversation
|
This relates to RPOPC-569 |
dvalinrh
left a comment
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.
LGTM
detect_os
Outdated
| elif command -v apt > /dev/null 2>&1; then | ||
| apt install -y wget | ||
| else | ||
| zypper install -y wget |
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.
Fix indent
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.
The file seems to have some inconsistent indents, some tabs, some spaces, some mixed. In vim it's actually lined up properly. I'll just cut-n-paste the apt line to get the right indentation but someone else can go fix anything else that's wonky.
detect_os
Outdated
| continue | ||
| fi | ||
|
|
||
| if [ -z pattern ]; then |
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.
Shouldn't this start with a $?
So this:
| if [ -z pattern ]; then | |
| if [ -z "$pattern" ]; then |
I think the uname pattern has the same problem
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 thought it looked wrong as well but as you said the uname pattern is wrong and I was just copying what was there assuming it was correct. Want me to fix both at the same time or leave the other so as to not mix issues in the same push?
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.
Go ahead and fix both, no reason to delay this small of a fix/bug
kdvalin
left a comment
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.
We should really remove the entirety of the package installation from this, to be discussed at a later meeting
detect_os
Outdated
| else | ||
| zypper install -y wget |
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.
Add error message
| else | |
| zypper install -y wget | |
| elif command -v zypper > /dev/null 2>&1; then | |
| zypper install -y wget | |
| else | |
| echo "Missing wget, and do not know what package manager is used, exiting out" | |
| exit 1 | |
| fi |
…r message if we can't figure out which package manager to use.
kdvalin
left a comment
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.
LGTM
Description
Add SLES as a valid target for detect_os
Before/After Comparison
Before: detect_os will error out on SLES systems that 'sles' is not a known OS and tests will abort
After: SLES is properly identified and tests run to completion
Clerical Stuff
This closes #84
Relates to JIRA: RPOPC-569