-
Notifications
You must be signed in to change notification settings - Fork 6
Feature/#31 add k8s resource to manage a k8s instance #41
Feature/#31 add k8s resource to manage a k8s instance #41
Conversation
…kubernetes instance
cf3f29c
to
8203203
Compare
@hieptranquoc please make sure we get all the tests passed on travis-ci. |
README.md
Outdated
@@ -40,6 +41,12 @@ helm 'install helm' do | |||
version '' #application version (if empty, default: latest) | |||
binary_path '' #application path (if empty, default: /usr/local/bin/helm) |
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 guess we need to update this
README.md
Outdated
@@ -164,7 +172,7 @@ end | |||
### Properties | |||
- `action` - `:install` to install `kubectl`, `:remove` to uninstall `kubectl`. | |||
- `version` - The desired version of `kubectl`. | |||
- `binary_path` - Application path (if empty, default:/usr/local/bin/kubectl) | |||
- `binary_path` - Application path (if empty, default:/usr/local/bin) |
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.
default:/usr... => default: /usr... (we need a white space after the semicolon character)
resources/kubernetes.rb
Outdated
end | ||
|
||
bash 'create kubectl config dir' do | ||
user 'vagrant' |
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.
this is hard-coded, make sure it's configurable
@hieptranquoc This is minikube install, let's call the resource: "minikube" instead of "kubernetes" because we can have more k8s deployment resource in the future. minikube is one type of k8s deployment which is intented for local dev only, this is basically good to me, just to adjust and update more basing on my comments and we should be good to go to release this for minikube resource. This should be very handful for our local dev testing. |
@phuonglm @datphan should look into this for your reference, this is the installer of k8s with minikube https://github.com/kubernetes/minikube |
something wrong with this |
this step is super slow, not sure why so super slow? |
and then I got the following errors several times:
|
resources/kubernetes.rb
Outdated
'HOME' => '/home/vagrant', | ||
'USER' => 'vagrant' | ||
) | ||
command "sudo -E minikube start --vm-driver=none --network-plugin=cni --bootstrapper=kubeadm --kubernetes-version #{k8s_version}" |
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 use #{binary_path}/minikube
resources/kubernetes.rb
Outdated
end | ||
|
||
action_class do | ||
def docker |
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.
for docker installation, please use https://github.com/chef-cookbooks/docker instead.
resources/kubernetes.rb
Outdated
end | ||
end | ||
|
||
def create_vagrant_user |
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.
vagrant is so specific to the vagrant user, this is not good, we should make sure to support any specified user instead.
I got this problem sometimes:
we need to make sure that the PR works as stable as possible. @hieptranquoc this takes a lot of time to get this done, please concentrate to get this done ASAP. |
the installation does not always work, it took me a lot of time to figure things out, sometimes it works, sometimes it does not, very unstable. |
I got repeatedly this kind of error:
|
seems that I'm having this problem: kubernetes/minikube#2707 make sure we can specify the minikube version. |
e8711de
to
599d741
Compare
3d0bad8
to
1e00ccc
Compare
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.
Please check my comments.
@@ -0,0 +1,16 @@ | |||
<?xml version="1.0" encoding="UTF-8"?> |
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.
git ignore .idea stuff
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.
remove this file
README.md
Outdated
- `minikube` should be run with special user. You can change by override attributes in recipe: | ||
|
||
```ruby | ||
node.override['kubernetes-stack']['user'] = `user` |
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.
node.override['kubernetes-stack']['minikube_user'] = "user" is better to me.
README.md
Outdated
node.override['kubernetes-stack']['user'] = `user` | ||
``` | ||
|
||
- or edit attributes in `default.rb` file |
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.
edit the vendor files should not be allowed.
attributes/default.rb
Outdated
@@ -1,21 +1,27 @@ | |||
# frozen_string_literal: true |
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.
wrong license, please use the previous Teracy license header.
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 don't think we need any attributes here. It does not make sense to me.
attributes/default.rb
Outdated
# See the License for the specific language governing permissions and | ||
# limitations under the License. | ||
# | ||
default['kubernetes-stack']['arch'] = 'amd64' |
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.
why this?
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.
this is dynamic and can be got from the chef provided info. Let users to override these kind of attributes is non-sense.
attributes/default.rb
Outdated
# | ||
default['kubernetes-stack']['arch'] = 'amd64' | ||
default['kubernetes-stack']['sys'] = 'linux' | ||
default['kubernetes-stack']['user'] = node['kubernetes-stack']['user'] || 'vagrant' |
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.
This is specific to a resource, move this to the resource property instead of the general cookbook attributes.
cookbook attributes can be used for some general or common config, don't overuse it here.
attributes/default.rb
Outdated
default['kubernetes-stack']['arch'] = 'amd64' | ||
default['kubernetes-stack']['sys'] = 'linux' | ||
default['kubernetes-stack']['user'] = node['kubernetes-stack']['user'] || 'vagrant' | ||
default['kubernetes-stack']['home'] = "/home/#{node['kubernetes-stack']['user']}" |
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 move to the minikube resource property, is this really needed for users to configure? Which case do users need to configure this?
attributes/default.rb
Outdated
default['kubernetes-stack']['home'] = "/home/#{node['kubernetes-stack']['user']}" | ||
|
||
default['docker_machine']['version'] = 'v0.14.0' | ||
default['docker_machine']['command_path'] = '/usr/local/bin/docker-machine' |
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.
path, use the same consistent name.
default['docker_machine']['path'] = '/usr/local/bin'
.kitchen.dokken.yml
Outdated
intermediate_instructions: | ||
- RUN yum -y install lsof which net-tools curl bash-completion | ||
|
||
# - name: centos-7 |
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.
why remove centos-7 tests?
resources/gcloud.rb
Outdated
|
||
install_path = '/usr/lib' | ||
log "create #{new_resource.path} directory" do |
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.
Why not this?
Chef::Log.info("create #{new_resource.path} directory")
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.
and why duplicated logs?
562a87f
to
c85747b
Compare
c85747b
to
ba89ab0
Compare
@hoatle i fixed all your comment, please check again! |
@@ -0,0 +1,16 @@ | |||
<?xml version="1.0" encoding="UTF-8"?> |
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.
remove this file
@@ -0,0 +1,4 @@ | |||
<?xml version="1.0" encoding="UTF-8"?> |
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.
remove all .idea/ files
@@ -19,3 +22,6 @@ | |||
# LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, | |||
# OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN | |||
# THE SOFTWARE. | |||
|
|||
default['docker_machine']['version'] = 'v0.14.0' |
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.
remove all these stuff, not useful to users and not supposed to be used by users.
end | ||
end | ||
|
||
action :run do |
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.
:run should not be introduced, we should have ":install", ":remove" action, that's enough for us.
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.
when minikube is stopped manually, :install should make it run again (this case is useful when I want to stop minikube temporarily and use it later)
@hieptranquoc please consider this: minikube "" do
version minikube_opt['version']
user_name minikube_opt['user_name']
action [:install, :remove]
end
minikube_k8s "" do
k8s_version
network_plugin
bootstrapper
vm_driver
action [:start, :stop]
end We have 2 resources: one to install/remove minikube and one to start/stop minikube_k8s. install action: install the exact specified minikube version or the latest one I think this will help us to manage minikube better. Let me know what you think then. |
Please use
Because of this error:
|
Same thing happen at
Because of this error:
|
close for now, we're moving to https://github.com/hoatle/teracy-dev-k8s instead. |
please help me check this PR! pls