Skip to content

Configure vagrant+ansible #325

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

Closed
wants to merge 1 commit into from
Closed

Configure vagrant+ansible #325

wants to merge 1 commit into from

Conversation

PI-Victor
Copy link

For: #268
Will implement as stated in #267
Signed-off-by: pi-victor ipalade@redhat.com

Review on Reviewable

Signed-off-by: pi-victor <ipalade@redhat.com>
@php-coder php-coder changed the title [WIP] Vagrant image Configure vagrant+ansible Jan 20, 2016
@php-coder
Copy link
Owner

Thanks for the PR. Just want to say that you don't need to prepend PR with [WIP] because there are not so many PRs and only one human who merges it :)

P.S. I also modified the description to make it clear what this PR is about.

@php-coder
Copy link
Owner

Reviewed 6 of 6 files at r1.
Review status: all files reviewed at latest revision, 15 unresolved discussions.


.gitignore, line 18 [r1] (raw file):
It looks a strange that you put comment at the end of the file and the excluded pattern at the beginning. Could you put the pattern after the comment?


vagrant/inventory, line 1 [r1] (raw file):
Do we really need this file?


vagrant/mystamps-base.yml, line 1 [r1] (raw file):
Just use all instead of concrete host name.


vagrant/mystamps-base.yml, line 3 [r1] (raw file):
Let' remove these for a now.


vagrant/mystamps-base.yml, line 6 [r1] (raw file):
Unused. Remove.


vagrant/mystamps-base.yml, line 9 [r1] (raw file):
Let's replace this with single task for showing message with simple text like "Provisioning MyStamps". See http://docs.ansible.com/ansible/debug_module.html


vagrant/roles/common/tasks/main.yml, line 6 [r1] (raw file):
We don't need to use role in this task. Also the role name is too broad.


vagrant/Vagrantfile, line 2 [r1] (raw file):
Let's remove these comments :)


vagrant/Vagrantfile, line 13 [r1] (raw file):
My approach is to use minimal config as possible and add things only when we really need them. Right now I don't see the reason of adding this. Let's remove them.


vagrant/Vagrantfile, line 17 [r1] (raw file):
Don't try to add things that we don't need yet.


vagrant/Vagrantfile, line 22 [r1] (raw file):
Hm... I have never need this and also I don't see same instructions here: http://docs.ansible.com/ansible/guide_vagrant.html#vagrant-setup

And I don't see this (https://www.vagrantup.com/docs/provisioning/ansible.html) too. Hm.. we don't need to have Ansible on the guest. We should have ansible in the host system.


vagrant/Vagrantfile, line 26 [r1] (raw file):
These 2 lined should be removed. We will end up with the super minimal Vagrantfile.


vagrant/Vagrantfile, line 27 [r1] (raw file):
Yeah, that the most useful line :)

Let's rename the file to mystamps-vagrant.yml


vagrant/Vagrantfile, line 28 [r1] (raw file):
We don't need inventory.


vagrant/vars/all, line 0 [r1] (raw file):
I don't think that we need it. At least right now. I suggest to remove this file.


Comments from the review on Reviewable.io

@PI-Victor
Copy link
Author

👍 i'll refactor accordingly, just so you know most of it were copied directly from my vagrant machine. i'll ping you when i updated it.

@PI-Victor
Copy link
Author

sorry man, i can't find the time to work on other projects anymore so i have to let this go.

@PI-Victor PI-Victor closed this Feb 20, 2016
@PI-Victor PI-Victor deleted the gh_268 branch February 20, 2016 22:53
@php-coder
Copy link
Owner

No problem! Thank you anyway!

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.

2 participants