-
Notifications
You must be signed in to change notification settings - Fork 37
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
Add Roles tasks and vars SLES #158
Conversation
Thank you for your pull request. Before we can look at it, you'll need to sign a Contributor License Agreement (CLA). Please follow instructions at https://icinga.com/company/contributor-agreement to sign the CLA. After that, please reply here with a comment and we'll verify. Contributors that have not signed yet: @dh-roland
|
Recheck |
@cla-bot check |
Thanks for the PR! We will have a look at it 👍 |
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.
Can you add a blank line between the tasks, it looks a bit cleaner and supports the readability.
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.
Additional can you please use the Ansible FQCN notation for the zypper modules. https://docs.ansible.com/ansible/latest/collections/community/general/zypper_module.html
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.
Thanks for the PR, eveything looks great so far. Two changes should be made, firstly the cosmetics in the install.yml. And secondly please add a changelog fragment.
Just create a yaml file in the changelog/fragments
folder. Naming scheme should be something like feature_things_change_at_installation.yml
and the content is similar to the following file: https://github.com/Icinga/ansible-collection-icinga/blob/main/changelogs/fragments/feature_monitoring_plugins.yml
Other than that, I'm happy to merge your PR
- changes for better reading
@mkayontour Can you please check again :) |
I merged the PR, thank you very much for your contribution! |
For issue: #157
Add Role Tasks and vars for SUSE