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

Darwin detect #349

Merged
merged 9 commits into from Jul 12, 2018

Conversation

@mmurphy
Copy link
Contributor

commented Jul 11, 2018

This is to address issue #314
There were a few changes required due to differences in commands on macOS.
There should be a better solution to the IP address issue then the option I coded which just tells the user to specify the command line argument.
It really should be tested on a "clean" macOS install, which I haven't done.
It works with the docker-machine setup described https://medium.com/@pilhuhn/running-openshift-with-istio-on-os-x-1d4e5014bd01, but not with Docker for Mac Version 18.03.1-ce-mac65 (24312). I didn't try other docker versions.

comments and reviews welcome, particularly if I've missed any project requirements.

mmurphy added 6 commits Jul 4, 2018
…s no specified

the 'ip' command is available on linux and is used to get the IP address of the last active IPV4 interface, this command is not normally available on mac
so require that the users specify the address to use
'-r' option for xargs not available on macos and -l would need to be used as '-L1'
@rhqci

This comment has been minimized.

Copy link
Collaborator

commented Jul 11, 2018

Can one of the admins verify this patch?

Copy link
Contributor

left a comment

Just asking for some minor changes. Looking good though. Almost there.

OPENSHIFT_IP_ADDRESS=${OPENSHIFT_IP_ADDRESS:-`echo $(ip -f inet addr | grep 'state UP' -A1 | tail -n1 | awk '{print $2}' | cut -f1 -d'/')`}
else
if [ ! "$OPENSHIFT_IP_ADDRESS" ] ; then
echo "ERROR: can't detect IP address please specify with --address argument"

This comment has been minimized.

Copy link
@jmazzitelli

jmazzitelli Jul 11, 2018

Contributor

Rather than abort and require the option, just default to 127.0.0.1 - that should be fine under most conditions (and it is usually OK for most people anyway to bind to the loopback address). So I would delete lines 185 and 186 and replace it with the line OPENSHIFT_IP_ADDRESS="127.0.0.1"

This comment has been minimized.

Copy link
@mmurphy

mmurphy Jul 11, 2018

Author Contributor

Sure I can do that, but the comment on line 180 says "NOTE: Do not use any IP address within the loopback range of 127.0.0.x."
I'll remove that also if you think it's incorrect

This comment has been minimized.

Copy link
@jmazzitelli

jmazzitelli Jul 11, 2018

Contributor

I don't know why that is in there... I think I was thinking if you use 127.x then you can't get to your cluster from another machine (which is why I originally added this option to change the IP address :)) I would remove it. I think it will work.

if [ "${DETECTED_OS_VERSION}" = "linux" -o "${DETECTED_OS_VERSION}" = "darwin" ]
then
DEFAULT_OS_VERSION=${DETECTED_OS_VERSION}
echo set default version to ${DEFAULT_OS_VERSION}

This comment has been minimized.

Copy link
@jmazzitelli

jmazzitelli Jul 11, 2018

Contributor

Is this asking the user to "set" the default version to something? Seems like the user is being told to set something. I would make the message more clear - don't make it seem like a command to the user to do something; log it as an informative message. Something like:

echo "The operating system has been detected as ${DEFAULT_OS_VERSION}"

This comment has been minimized.

Copy link
@mmurphy

mmurphy Jul 11, 2018

Author Contributor

I agree, it is confusing, probably better to use your text as set it as a debug message only

DOWNLOADER="curl -L -o"
fi
fi
if [ ! "$DOWNLOADER" ] ; then

This comment has been minimized.

Copy link
@jmazzitelli

jmazzitelli Jul 11, 2018

Contributor

Do not abort here if curl or wget is not found - it is possible there is no need to download anything (perhaps the user already downloaded it in a previous run manually?). I would move this down to above line 320, inside the if-stmt when you know you are going to need the DOWNLOADER var.

This comment has been minimized.

Copy link
@mmurphy

mmurphy Jul 11, 2018

Author Contributor

agreed

@@ -289,7 +317,7 @@ if [[ -f "${OS_ISTIO_OC_EXE_PATH}" ]]; then
fi
else
echo "Downloading binary to ${OS_ISTIO_OC_EXE_PATH}"
wget -O ${OS_ISTIO_OC_EXE_PATH} ${OS_ISTIO_OC_DOWNLOAD_LOCATION}
eval ${DOWNLOADER} ${OS_ISTIO_OC_EXE_PATH} ${OS_ISTIO_OC_DOWNLOAD_LOCATION}

This comment has been minimized.

Copy link
@jmazzitelli

jmazzitelli Jul 11, 2018

Contributor

Just above this line is where you can check if DOWNLOADER is valid and if not then abort.

DETECTED_OS_VERSION=`uname | tr '[:upper:]' '[:lower:]'`
if [ "${DETECTED_OS_VERSION}" = "linux" -o "${DETECTED_OS_VERSION}" = "darwin" ] ; then
DEFAULT_OS_VERSION=${DETECTED_OS_VERSION}
debug "he operating system has been detected as ${DEFAULT_OS_VERSION}"

This comment has been minimized.

Copy link
@josejulio

josejulio Jul 11, 2018

Member

Missing the T for The

This comment has been minimized.

Copy link
@mmurphy

mmurphy Jul 11, 2018

Author Contributor

@josejulio Thanks, I really hate that kind of silly error. Is it preferred to squash that fix with the previous commit?

@@ -289,7 +307,22 @@ if [[ -f "${OS_ISTIO_OC_EXE_PATH}" ]]; then
fi
else
echo "Downloading binary to ${OS_ISTIO_OC_EXE_PATH}"
wget -O ${OS_ISTIO_OC_EXE_PATH} ${OS_ISTIO_OC_DOWNLOAD_LOCATION}

# Use curl command if available, otherwise try wget

This comment has been minimized.

Copy link
@josejulio

josejulio Jul 11, 2018

Member

Minor, but I think this should say

# Use wget command if available, otherwise try curl

Because is trying wget first, if not using curl (no the other way)

This comment has been minimized.

Copy link
@mmurphy

mmurphy Jul 11, 2018

Author Contributor

agreed

@jmazzitelli

This comment has been minimized.

Copy link
Contributor

commented Jul 11, 2018

Trying on my linux box where I already have istiooc downloaded. I am getting this warning - looks like the version check is broken - notice the current version shows as a empty string ("The version of the client binary is:").

===== WARNING =====
You already have the client binary but it does not match the version you want.
Either delete your existing client binary and let this script download another one,
or change the version passed to this script to match the version of your client binary.
Client binary is here: /home/mazz/bin/istiooc
The version of the client binary is: 
You asked for version: istio-3.9-0.8.0-alpha4
===== WARNING =====
@jmazzitelli

This comment has been minimized.

Copy link
Contributor

commented Jul 11, 2018

Here's what I see:

$ /home/mazz/bin/istiooc --request-timeout=2s version | head -n 1
oc istio-3.9-0.8.0-alpha3+adb17ef

It appears the new sed expression broke - it can no longer extract the version string.

Because when I pipe to sed it returns empty string:

$ /home/mazz/bin/istiooc --request-timeout=2s version | head -n 1 | sed -n "s/^oc \([A-Za-z0-9.-]*\)\+[a-z0-9 ]*$/\1/p"
$
@@ -275,7 +293,7 @@ fi

# Download the oc client if we do not have it yet
if [[ -f "${OS_ISTIO_OC_EXE_PATH}" ]]; then
_existingVersion=$(${OS_ISTIO_OC_EXE_PATH} version | head -n 1 | sed -n "s/^.* \(\S*\)+.*$/\1/p")
_existingVersion=$(${OS_ISTIO_OC_EXE_PATH} --request-timeout=2s version | head -n 1 | sed -n "s/^oc \([A-Za-z0-9.-]*\)\+[a-z0-9 ]*$/\1/p")

This comment has been minimized.

Copy link
@jmazzitelli

jmazzitelli Jul 11, 2018

Contributor

This new sed expression is broken. The old one still works:

$ /home/mazz/bin/istiooc --request-timeout=2s version | head -n 1 | sed -n "s/^.* \(\S*\)+.*$/\1/p"
istio-3.9-0.8.0-alpha3
$
Copy link
Contributor

left a comment

See my previous comments - line 296 should only add the new "--request-timeout=2s" - everything else should stay the same (specifically the sed expression should remain as it was before).

@jmazzitelli

This comment has been minimized.

Copy link
Contributor

commented Jul 11, 2018

FYI: all my other testing passes. If I revert to the old sed expression, this all works on linux (tried both up and down).

@mmurphy

This comment has been minimized.

Copy link
Contributor Author

commented Jul 11, 2018

@jmazzitelli

This comment has been minimized.

Copy link
Contributor

commented Jul 11, 2018

istiooc version output is below:

oc istio-3.9-1.0.0-alpha1+a8f76d5
kubernetes v1.9.1+a0ce1bc657
features: Basic-Auth GSSAPI Kerberos SPNEGO

Server https://192.168.1.19:8443
openshift v3.9.0+71543b2-33
kubernetes v1.9.1+a0ce1bc657
@jmazzitelli

This comment has been minimized.

Copy link
Contributor

commented Jul 11, 2018

@mmurphy feel free to have different sed expressions per "linux" vs. "darwin" - now that you have a var. that tells you if we are darwin or linux, you can have an if...else to set a new "SED_VERSION_EXPR" that is different for linux (the old expression) or darwin (the new one you are using).

if it is then run it in posix mode to be compatible with standard sed on macOS
It's done this way instead of checking darwin so that it will also work on macOS if gnu-sed in installed as 'sed'
@mmurphy

This comment has been minimized.

Copy link
Contributor Author

commented Jul 11, 2018

I've added a detector for gnu-sed, since it could be installed on macOS with
brew install gnu-sed --with-default-names
so it's better to check if sed is gnu or not

@jmazzitelli

This comment has been minimized.

Copy link
Contributor

commented Jul 11, 2018

This works for me on linux. Anyone else want to test on MacOS before we merge? I do not have a Mac to test on. @pilhuhn ?

Let's give this until tomorrow to give people a chance to test, but I think we can merge this tomorrow if no one objects.

Thanks @mmurphy !!

@jmazzitelli jmazzitelli merged commit e2a4a90 into kiali:master Jul 12, 2018
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@mmurphy mmurphy deleted the mmurphy:darwin_detect branch Jul 12, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.