-
Notifications
You must be signed in to change notification settings - Fork 16
Fix #350: Add support of 'install-cli' command for kubernetes #369
Conversation
* Refactored code to delegate execution code into respective service hooks
describe ADBKubernetesBinaryHandler do | ||
before do | ||
@machine = fake_machine | ||
@options = { type: :kubernetes, '--cli-version' => '1.2.0' } # 1.2.0 is available in CDK/ADB VM |
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 1.2.0 be hardcoded here? I believe this information is somewhere else too. I am cool with having an issue opened that notes we need to re-factor this, it appropriate so as to not hold this PR up.
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.
@bexelbie I think we can manage it well with using locale based or some single configuration based file.
I would like to make it part of refactoring as it should be done across all unit tests 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.
Cool - do you mind opening an issue on it so we don't lose it or your thoughts that you have right now? This should not block this PR.
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've left two comments inline that can be turned into issues and should not block this PR. However, I don't see the code that creates the config file for kubectl as referenced in https://gist.github.com/praveenkumar/90704a42fe857e1146f3dae2fcae69ef Am I missing it? |
@budhrg you mentioned in IRC that kube config creation is part of a separate PR. Where is that PR? I believe this code needs to be dependent on kubectl working once downloaded/installed. |
@bexelbie It is not dependent on the other PR since its an individual feature in itself. |
@budhrg I agree it is not dependent in a technical sense. However, from a user perspective we either need to tell them how to build their own config file in documentation or we need to add the code that generates the config and installs it for them. I am suggesting the dependency is usability based. |
@bexelbie , @navidshaikh : I have updated the PR with another commit to configure kube config and showing its path through Note: To verify this patch, one need to use this patched box. User workflow would be :
Below is my end-to-end kubernetes workflow logs
|
* generate kube's config as part of vagrant up process * updated config validation to ensure only one of openshift and kubernetes service is running * added acceptance test for kubernetes service(ADB/CDK) * updated env-command to reflect kubernetes env help
retest this please |
The code visually reviews well. I do not have time to test it today, unfortunately. @navidshaikh @tkral or @praveenkumar can you get to it today/your tomorrow morning? |
@budhrg I tested with latest CDK and kubeconf generated as expected and also binary was installed without any issue. LGTM Let's wait for @navidshaikh test. |
* Also excluded plugin_util file from rubocop check
Working in Windows 10 too. |
Working for Mac and Linux too. |
LGTM |
@navidshaikh / @bexelbie Can you merge it if that is working as expected? |
Fix #350
Also includes:
config.servicemanager.services = 'kubernetes
For quick test at local one can use
bundle exec rake test rubocop features
.My logs: