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

Updated README file in manage-credentials-types Ansible Tower role to better reflect variables in defaults. #484

Merged
merged 5 commits into from
Jul 28, 2020

Conversation

kenhitchcock
Copy link
Contributor

What does this PR do?

Brief explanation of the code or documentation change you've made.
Role now includes the following.

  • A proper README file with variables explained, along with how the playbook and inventory should be configured to complete a successful deployment.

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

… 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 | |
Copy link
Contributor

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.

@kenhitchcock
Copy link
Contributor Author

@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?

@oybed
Copy link
Contributor

oybed commented Jul 13, 2020

@kenhitchcock think there's some cross-talk here... Let me try to clarify:

  1. My earlier comment is strictly for your added lines/table
  2. These are values that apply to all the roles and these are already documented in the README one level up
  3. the default values don't need to be documented

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.

@kenhitchcock
Copy link
Contributor Author

@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.
Copy link
Contributor

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.  

@kenhitchcock
Copy link
Contributor Author

@oybed updated README as requested.

Thanks again for the guidance on these changes.

Copy link
Contributor

@oybed oybed left a comment

Choose a reason for hiding this comment

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

LGTM

@oybed oybed merged commit 3b5062b into redhat-cop:master Jul 28, 2020
@kenhitchcock kenhitchcock deleted the manage-credentials-types branch August 4, 2020 14:22
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