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

Adding comments and sensible defaults to all.sample #25

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

syed
Copy link
Contributor

@syed syed commented Apr 10, 2017

Hi @PaulAngus . As per our conversation last Friday, I am adding more comments in the all.sample file with some defaults. Let me know if I got them all right

@syed
Copy link
Contributor Author

syed commented Apr 12, 2017

I've also added SSH keys support so when you spin up VMs, Trillian will automatically add its ssh keys so that ansible can work. This is required for any public images as the root account is disabled for most of them.

you will have to specify -u centos -s when running ansible-playbook

Copy link
Member

@DaanHoogland DaanHoogland left a comment

Choose a reason for hiding this comment

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

@syed it turns out I don't have rights to merge. I chase getting those. In the meanwhile; I see a lot of hardcoding of values that used to be placeholders. Does it make sense to move those "defaults" to comments, to discourage their use?

def_env_use_sec_groups: yes
def_env_prihost: "<IP_ADDRESS_HERE>"
def_env_sechost: "<IP_ADDRESS_HERE>"
def_env_use_sec_groups: yes # BEING DEPRECATED:
Copy link
Member

Choose a reason for hiding this comment

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

did something go wrong here?

# BEING DEPRECATED:
def_env_zone_secgroups: "false"

def_env_zone_secgroups: "false" # BEING DEPRECATED:
Copy link
Member

Choose a reason for hiding this comment

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

or here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@DaanHoogland This is me basically reordering the comment. As per my discussion with Paul, he mentioned that these were being removed. However the placement of that comment made me think that everything after that was be depricated so I moved it to just that line that was being depricated.

@syed
Copy link
Contributor Author

syed commented Jun 9, 2017

@DaanHoogland @PaulAngus anything needed from my side to merge this?

@DaanHoogland
Copy link
Member

H @syed, yes. conflict resolution and the undoing of the changes in the sample file where place holders are filled with real values.

@syed
Copy link
Contributor Author

syed commented Jun 9, 2017

Ok. I will get to that this weekend :)

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