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

Issue 45 #47

Merged
merged 6 commits into from
Oct 28, 2022
Merged

Issue 45 #47

merged 6 commits into from
Oct 28, 2022

Conversation

afeefghannam89
Copy link
Member

Fix #45

These are some changes which suit the redis usage of this role.

* Enable setting permissions on certificate directory
* Enable defining certificate path on clients
* Enable defining key usage, default non because of redis
* Update docs
Copy link
Member

@widhalmt widhalmt left a comment

Choose a reason for hiding this comment

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

More or less my request can be summed up as "please prefix all variables with the name of the role". If there's a good reason you don't want that here, that might be ok for. e.g. when you need variables for interaction between roles.

if you want to accept my suggestion, you can make your life easier by just running sed -i 's/client_ca/ca_client_ca/g' filename on every file you changed and pushing that. I'll happily review it again.

README.md Outdated Show resolved Hide resolved
defaults/main.yml Outdated Show resolved Hide resolved
defaults/main.yml Outdated Show resolved Hide resolved
tasks/main.yml Outdated Show resolved Hide resolved
tasks/main.yml Outdated Show resolved Hide resolved
tasks/main.yml Outdated Show resolved Hide resolved
tasks/main.yml Outdated Show resolved Hide resolved
tasks/main.yml Outdated Show resolved Hide resolved
tasks/main.yml Outdated Show resolved Hide resolved
tasks/main.yml Outdated Show resolved Hide resolved
@afeefghannam89
Copy link
Member Author

@widhalmt Thanks for the hint and the help. Your suggested changes have been done.

Copy link
Member

@widhalmt widhalmt left a comment

Choose a reason for hiding this comment

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

Thanks for the code. This will make the role way more flexible!. Looks very good to me.

@afeefghannam89 afeefghannam89 merged commit dc8ac1e into main Oct 28, 2022
@afeefghannam89 afeefghannam89 deleted the issue-45 branch October 28, 2022 07:32
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.

Disable passphrase and enable setting not root owner on certificate directory
2 participants