-
Notifications
You must be signed in to change notification settings - Fork 949
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
Conversation
0c37447
to
f0cb4ba
Compare
Codecov Report
@@ 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
|
awsome works, can your please review this pr @fuweid thanks a lot? |
hack/vagrant/README.md
Outdated
@@ -0,0 +1,12 @@ | |||
# USAGE |
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.
Could you mind to add more information about Vagrant? For example, the official doc link.
How do you think?
hack/vagrant/bootstrap.sh
Outdated
set -eo pipefail | ||
shopt -s nullglob | ||
|
||
GOVERSION=1.9.2 |
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.
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?
hack/vagrant/bootstrap.sh
Outdated
|
||
# 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 |
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 shell command is too long. We can use the following script to make it readable
apt-get install -y git \
libncurses5-dev \
.... \
hack/vagrant/Vagrantfile
Outdated
config.vm.provision :shell, path: "bootstrap.sh" | ||
config.vm.synced_folder "../../", "/go/src/github.com/alibaba/pouch" | ||
|
||
config.vm.define "node0" do |node0| |
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.
node0
looks like random composed name. Can you change it to the pouch-dev-node
something like that?
hack/vagrant/Vagrantfile
Outdated
|
||
Vagrant.configure("2") do |config| | ||
config.vm.boot_timeout=600 | ||
config.vm.box = "bento/ubuntu-16.04" |
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.
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.
59cf318
to
20478a0
Compare
hack/vagrant/Vagrantfile
Outdated
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" |
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.
Hi, why we need this ip, here? we can use vagrant ssh
to login, right?
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.
Yes. Set static ip maybe helpful for debuging/reporting problems across users.
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.
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?
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.
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.
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.
Sure thing!!
install_docker(){ | ||
curl -fsSL get.docker.com -o get-docker.sh | ||
sh get-docker.sh | ||
} |
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.
add the blank line here :)
LGTM. But please use Ping @u2takey |
Signed-off-by: u2takey <[email protected]>
@fuweid thanks, signed. |
@u2takey thank you! Welcome to continue to contribute pouchd :) |
Ⅰ. Describe what this PR did
add vagrant environment for development
Ⅱ. Does this pull request fix one issue?
#1243