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

Allow optional variable to use inventory_hostname instead of ansible_hostname #38

Merged
merged 3 commits into from
Jul 30, 2022

Conversation

ObjectiveTruth
Copy link
Contributor

This PR includes a README to explain the issue that I encountered with this role.

I've dded an optional parameter(that defaults to false to preserve current behaviour) where the hostname used in the server list is used. This happens when the the host only gives its hostname instead of its FQDN(Fully Qualified Domain Name). This can cause the server being unable to see each other.

Also, this could be required if you want/need to use TLS since the hostname needs to match the certificate it presents

@ObjectiveTruth
Copy link
Contributor Author

Partially addresses #27

@ObjectiveTruth
Copy link
Contributor Author

Fixed up the README linting issues reported using force push

@ObjectiveTruth ObjectiveTruth force-pushed the master branch 4 times, most recently from c888228 to d208ab6 Compare July 26, 2022 12:56
@ObjectiveTruth
Copy link
Contributor Author

I enabled the linter on my fork, so the new forced push updates should pass all the tests this time 👍

@ObjectiveTruth
Copy link
Contributor Author

Can this be merged, @sleighzy?

README.md Outdated Show resolved Hide resolved
defaults/main.yaml Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@sleighzy
Copy link
Owner

Can this be merged, @sleighzy?

Hi @ObjectiveTruth . I've dropped a couple of comments on this now, and a request for slightly renaming the variables. I'll merge it in once you push the changes.

Thanks heaps by the way, much appreciated for the efforts and updates. Will be very helpful given this problem has come up before.

@ObjectiveTruth ObjectiveTruth force-pushed the master branch 2 times, most recently from a04b3fb to 3187380 Compare July 28, 2022 11:50
defaults/main.yaml Outdated Show resolved Hide resolved
@ObjectiveTruth
Copy link
Contributor Author

I've applied the updates and you're right the tests were failing because I missed the variable name change in the defaults/main.yml should be good to merge now

@sleighzy sleighzy merged commit 4c0dd89 into sleighzy:master Jul 30, 2022
@sleighzy
Copy link
Owner

Thanks @ObjectiveTruth , all sorted now, cheers for the contribution.

@ObjectiveTruth
Copy link
Contributor Author

Thanks @ObjectiveTruth , all sorted now, cheers for the contribution.

No problem! Glad i could help 👍

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