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

feature: add vagrant environment for development #1245

Merged
merged 1 commit into from
Apr 28, 2018

Conversation

u2takey
Copy link
Contributor

@u2takey u2takey commented Apr 27, 2018

Ⅰ. Describe what this PR did

add vagrant environment for development

Ⅱ. Does this pull request fix one issue?

#1243

@codecov-io
Copy link

codecov-io commented Apr 27, 2018

Codecov Report

Merging #1245 into master will increase coverage by 0.03%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1245      +/-   ##
==========================================
+ Coverage   15.27%   15.31%   +0.03%     
==========================================
  Files         172      172              
  Lines       10691    10665      -26     
==========================================
  Hits         1633     1633              
+ Misses       8938     8912      -26     
  Partials      120      120
Impacted Files Coverage Δ
cli/cli.go 0% <0%> (ø) ⬆️
daemon/config/config.go 10% <0%> (+2.1%) ⬆️

@HusterWan HusterWan requested a review from fuweid April 27, 2018 14:57
@HusterWan
Copy link
Contributor

awsome works, can your please review this pr @fuweid thanks a lot?

@@ -0,0 +1,12 @@
# USAGE
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you mind to add more information about Vagrant? For example, the official doc link.

How do you think?

set -eo pipefail
shopt -s nullglob

GOVERSION=1.9.2
Copy link
Contributor

Choose a reason for hiding this comment

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

In order to make the shell script readable, we can use function to wrap some common functions.

Let's make it clear and it's good for maintainers or contributors. Does it make senses?


# install git/libncurses5-dev/...
apt-get update
apt-get install -y git libncurses5-dev libslang2-dev gettext zlib1g-dev libselinux1-dev debhelper lsb-release pkg-config po-debconf autoconf automake autopoint libtool
Copy link
Contributor

Choose a reason for hiding this comment

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

This shell command is too long. We can use the following script to make it readable

apt-get install -y git \
    libncurses5-dev \
    .... \

config.vm.provision :shell, path: "bootstrap.sh"
config.vm.synced_folder "../../", "/go/src/github.com/alibaba/pouch"

config.vm.define "node0" do |node0|
Copy link
Contributor

Choose a reason for hiding this comment

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

node0 looks like random composed name. Can you change it to the pouch-dev-node something like that?


Vagrant.configure("2") do |config|
config.vm.boot_timeout=600
config.vm.box = "bento/ubuntu-16.04"
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we use the official image?
BTW, please offer the kernel version, since the containerd requires the higher kernel version. If you can offer the information, it maybe help contributor to diagnose.

@pouchrobot pouchrobot added size/L and removed size/M labels Apr 28, 2018
@allencloud allencloud changed the title add vagrant environment for development feature: add vagrant environment for development Apr 28, 2018
@u2takey u2takey force-pushed the master branch 2 times, most recently from 59cf318 to 20478a0 Compare April 28, 2018 03:20
config.vm.synced_folder "../../", "/go/src/github.com/alibaba/pouch"

config.vm.define "pouch-dev-node" do |node|
node.vm.network "private_network", ip: "172.30.30.10"
Copy link
Contributor

Choose a reason for hiding this comment

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

Hi, why we need this ip, here? we can use vagrant ssh to login, right?

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. Set static ip maybe helpful for debuging/reporting problems across users.

Copy link
Contributor

Choose a reason for hiding this comment

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

Basically, the static IP address is not good. It could cause conflict. If all the envs are the same, that will be fine. However, every dev-env is unique and under different sub-net.

Therefore, I think we can remove this configure. VirtualBox supports port forwarding so that we can use this to do things which you mentions.

How do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right, i'll remove this configure and keep it simple. It can be added if then we need a cluster setting like https://github.com/docker/libnetwork/blob/master/Vagrantfile or https://github.com/pires/kubernetes-vagrant-coreos-cluster/blob/master/Vagrantfile.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure thing!!

install_docker(){
curl -fsSL get.docker.com -o get-docker.sh
sh get-docker.sh
}
Copy link
Contributor

Choose a reason for hiding this comment

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

add the blank line here :)

@fuweid
Copy link
Contributor

fuweid commented Apr 28, 2018

LGTM. But please use git commit -s to add your signed-off. :)

Ping @u2takey

@fuweid fuweid assigned fuweid and unassigned fuweid Apr 28, 2018
Signed-off-by: u2takey <u2takey@gmail.com>
@u2takey
Copy link
Contributor Author

u2takey commented Apr 28, 2018

@fuweid thanks, signed.

@fuweid fuweid merged commit 771a167 into AliyunContainerService:master Apr 28, 2018
@fuweid
Copy link
Contributor

fuweid commented Apr 28, 2018

@u2takey thank you! Welcome to continue to contribute pouchd :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants