-
Notifications
You must be signed in to change notification settings - Fork 122
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
Updated README file in manage-credentials-types Ansible Tower role to better reflect variables in defaults. #484
Updated README file in manage-credentials-types Ansible Tower role to better reflect variables in defaults. #484
Conversation
… better reflect variables in defaults file. As per issue redhat-cop#400
@@ -12,6 +12,10 @@ A running Ansible Tower with admin permission level access. | |||
|
|||
The variables used to maintain the Credential Types must be defined in the Ansible Inventory using the `ansible_tower.credential_types` dictionary as explained below. | |||
|
|||
| Variable | Description | Required | Defaults | | |||
|:---------|:------------|:---------|:---------| | |||
|**default_ansible_tower_url**| Ansible Tower url | yes | | |
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.
I'd recommend that we point to the README at the roles/ansible/tower
level for these as the default
values are truly just that - i.e.: default values - and the values should be supplied as described at the above mentioned README.
@oybed i only added this readme file as the other sub-roles had a README and thought this one should have one too. If you want this one removed then does that mean we should remove the others too? |
@kenhitchcock think there's some cross-talk here... Let me try to clarify:
In other words, I don't see the need for this change as-is, and the only change that may be good is a pointer to the above Example Inventory as these values are already explained there. |
@oybed, ok understood. I will remove the entry and just mention in the README to look at the master README file for more details if that's ok? |
@@ -34,6 +33,9 @@ ansible_tower: | |||
id: "<REPLACE WITH CORRESPONDING CREDENTIAL TYPE ID>" | |||
``` | |||
|
|||
A more detailed explanation of the variables required for this role can be found in the parent README.md found at the link below. |
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.
Please change to:
Also check the [README one level up](../README.md#example-inventory) for additional details of an example inventory.
@oybed updated README as requested. Thanks again for the guidance on these changes. |
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.
LGTM
What does this PR do?
Brief explanation of the code or documentation change you've made.
Role now includes the following.
How should this be tested?
Automated tests are preferred, but not always doable - especially for infrastructure. Include commands to run your new feature, and also post-run commands to validate that it worked. (please use code blocks to format code samples)
Just an admin update, no functional testing required, only proof read.
Is there a relevant Issue open for this?
Provide a link to any open issues that describe the problem you are solving.
resolves #
The issue I believe is related to #400
Other Relevant info, PRs, etc.
Please provide link to other PRs that may be related (blocking, resolves, etc. etc.)
People to notify
cc: @redhat-cop/infra-ansible
@redhat-cop/infra-ansible
@oybed