-
Notifications
You must be signed in to change notification settings - Fork 0
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
first script idea #72
Conversation
ok. For me the script looks ok. Feel free to review :) |
@daniel-wtd feel free to review :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Cinux90 I had a review and it looks quite ok. I recommend the following steps:
- remove the "create role stuff" -> currently it's only 2 steps
- check with a linter like flake8
- the ansible cfg seems to be written relative to the execution path, someone can have other stuff in there or multiple configurations and we should avoid directly manipulate such stuff. it's better to output the needed steps at the end.
- a little bit more documentation in the code would be awesome at least each
def
should be documented to use some auto-doc tools in a later process
Otherwise, cool work
ty for review. If the weather will be colder i will have a look.. ;) |
stale for ages -> remove |
resolve: #63
currently todo:
@while-true-do/ansible-developer: Feel free to bring ideas