search

mrlesmithjr / ansible-consul

Ansible role to install/configure Consul
MIT License
8 stars 5 forks source link

Fix vagrant folder #21

Closed jardleex closed 7 years ago

jardleex commented 7 years ago

Hello @mrlesmithjr,

during my work with the role I noticed that inside the Vagrant folder roles/ansible-consul was copied multiple times into itself. This PR fixes this and set's a symlink to the top level role. It also fixes an .append issue in templates/etc/consul.d/client/service.json.j2 as found in #14. So both PR are fixing the Vagrant box.

Best regards

Jard

mrlesmithjr commented 7 years ago

Good catch. This is from the way that I add roles for Vagrant. I will look at a different way to do this.

jardleex commented 7 years ago

Okay, the symlink was my intuitive way of solving the recursion. Let me know if you prefer another way.

mrlesmithjr commented 7 years ago

@jardleex I have cleaned up the nested folders here. Can you rebase?

jardleex commented 7 years ago

I'm afraid that I'm stuck here. My changes are that "old" and I'm not sure how to do this rebase as I throws errors and errors to me as so much change in the meantime.

Nonetheless I checked you fixes/#21 branch and saw that we had different approaches on this. I replaced Vagrant/roles/ansible-consul with a symlink ../../ which points to the role base directly. This would give the user the most recent role version to test in Vagrant. The intention behind that was also to ease the Vagrant setup and not beeing forced to port each role change into Vagrant as well.

Seems that you follow the approach to keep Vagrant/roles/ansible-consul as independent folder.

As it is your role it's up to you to decide which approach you would like to use.

mrlesmithjr commented 7 years ago

Actually, I do like your approach much better.

mrlesmithjr commented 7 years ago

Merged