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

Use locally installed oc if possible #107

Closed
wants to merge 2 commits into from

Conversation

ifireball
Copy link
Contributor

Determine if the right version of the oc binary is installed in $PATH and use it if it does.

@adl-bot
Copy link

adl-bot commented Nov 4, 2018

Can one of the admins verify this patch?

@arilivigni
Copy link
Member

We still want to download the client in the default state. That's why this was put here since people complained that they have to download the client themselves. Did you put a conditional around it? We're not going to remove that logic.

@ifireball
Copy link
Contributor Author

Local oc is only used if found on $PATH and matches the version requested in thew configuration - otherwise the previous downloading logic runs.

@arilivigni
Copy link
Member

[test]

@arilivigni
Copy link
Member

Hard to read on my phone I'll have to look later. In the meantime we can test it

@ifireball
Copy link
Contributor Author

Looks like I have a bug, will fix

Determine if the righe version of the `oc` binary is installed in
`$PATH` and use it if it does.

This resolves issue CentOS-PaaS-SIG#102.

Signed-off-by: Barak Korren <[email protected]>
Copy link
Member

@arilivigni arilivigni left a comment

Choose a reason for hiding this comment

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

I honestly don't want this change I would rather just have a skip oc install just like minishift does is better than putting in this logic -1 from me

command: "{{ oc_bin }} login {{ cluster_ip|quote }} --username={{ username|quote }} --password={{ password|quote }} --insecure-skip-tls-verify=true"
Copy link
Member

Choose a reason for hiding this comment

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

Will this login maintain within the shell if done this way for the next call?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes. its seems its stored in a file somewhere, so it does not even matter which binary you've used to login.
Also seems to work across oc versions - but that is probably not guaranteed.

@arilivigni arilivigni requested a review from dirgim November 5, 2018 15:54
Copy link
Contributor

@dirgim dirgim left a comment

Choose a reason for hiding this comment

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

I think it would be better to just have an option to skip the oc setup - the path to the oc binary can already be supplied from the command line if the user wants to use the locally installed one.

@arilivigni
Copy link
Member

+1 to @dirgim that was similar to my comment just override and skip the install of the oc client

@ifireball
Copy link
Contributor Author

@dirgim @arilivigni I would like to try and avoid adding yet another configuration variable given that we already have oc_bin.

How about if I check for existence, executability and version of the path oc_bin point to - and skip downloading if the test succeeds?

This way:

  1. I can set it to simply oc on my system to get the behaviour I want
  2. It will actually avoid downloading the 2nd time someone runs the script while still being able to download the 1st time

There is one thing Ill need to address which is what happens if the binary is in place but of the wrong version - I think I'll just download to a different location in that case (I wouldn't want to just blindly override it)

@arilivigni
Copy link
Member

@ifireball I don't see the big deal if you have your options defined in a file adding one more does not mean you need it on the command line. I think we are over complicating things here for a simple option override that still give you what you want. I would be a -1 on this change.

@arilivigni
Copy link
Member

@ifireball an even easier solution is to symlink your version into the normal location to minishift and we can put a check to see if the version is there and then not download it. This avoids an extra option and avoids the time to download

@ifireball
Copy link
Contributor Author

ifireball commented Nov 6, 2018

@arilivigni just to make sure I fully understand what you're saying - you opposed to any form of automated checking for wither oc is in place, and instead prefer if we just have a configuration options where we say that.

(Fine by me, I just want to understand...)

@arilivigni
Copy link
Member

@ifireball I think it is fine to check if it is downloaded and not download the same version again. In your case a symlink would suffice. This way you download it once for the version that needs to be used. Server and client should match.

@arilivigni
Copy link
Member

There is a PR with a better approach in the works

@arilivigni arilivigni closed this Jan 8, 2019
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.

4 participants