-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
BZ1292961 Documenting Ansible options in Install Guide #2350
BZ1292961 Documenting Ansible options in Install Guide #2350
Conversation
aacb6fa
to
4b05f70
Compare
4b05f70
to
76648fe
Compare
6685862
to
08c2ea2
Compare
08c2ea2
to
c0cb9fc
Compare
@@ -16,8 +16,7 @@ Administrators can configure custom serving certificates for the public host | |||
names of the {product-title} API and | |||
xref:../architecture/infrastructure_components/web_console.adoc#architecture-infrastructure-components-web-console[web console]. | |||
This can be done during an | |||
xref:../install_config/install/advanced_install.adoc#advanced-install-custom-certificates[advanced | |||
installation] or configured after installation. | |||
xref:../install_config/install/advanced_install.adoc#advanced-install-custom-certificates[advanced installation] or configured after installation. |
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 like this, however don't feel it call's out "loudly" enough that we should be using Ansible to configure these directives, when possible. I wonder if duplicating content (as a new section) on this document, from the link provided helps to make this point?
- Note: I am not a fan of duplicating content.
I like what was done in https://github.com/openshift/openshift-docs/pull/2350/files#diff-2286e6c0ed74dfb182205830dcec2932R29 as it summarizes an example using the links.
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 don't like duplicating content either unless absolutely necessary (and if I did, I'd use "include" statements so the text wouldn't be in there redundantly). I've made this a new section which is closer to what I did for other topics, let me know if you feel this satisfies what you were looking for.
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.
This new section does exactly what I was hoping we could do. Looks good.
c0cb9fc
to
85f856b
Compare
Updates made, PR updated, nice rendered docs here if you like: http://file.bne.redhat.com/tpoitras/2016/ansiblehosts/openshift-enterprise/ansibleflaginhosts-BZ1292961/install_config/index.html What do you reckon, @sferich888 ? Ack? |
@@ -13,8 +13,7 @@ toc::[] | |||
== Overview |
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.
In talking this over with @TheDiemer he pointed out that the titles / section headers should read:
Configuring OpenShift Enterprise [COMPONENT] for PROVIDER/PLATFORM
As its more accurate to what we are trying to configure.
though it can be | ||
xref:../install_config/install/advanced_install.adoc#configuring-ansible[overridden | ||
during installation] using the `*os_sdn_network_plugin_name*` parameter. | ||
== Configuring the Pod Network with Ansible |
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.
Needs ID
85f856b
to
d1cc3c1
Compare
Thank you @sferich888 and @adellape |
LGTM |
== Configuring Masters | ||
== Configuring {product-title} Masters for AWS | ||
|
||
You can set AWS configuration on your {product-title} masters two ways: |
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.
"You can set the AWS configuration on your {product-title} master hosts in two ways:"
@tpoitras Some comments from me above. Overall, looks good. |
@bfallonf Thanks so much! 🎉 ☀️ 🔧 |
d1cc3c1
to
ca4fd6d
Compare
[rev_history] |
@tpoitras - this shouldn't still be on peer review. |
Thanks @vikram-redhat |
@tpoitras This PR makes changes to We might be able to use something like this to avoid these sorts of issues: https://git-scm.com/book/en/v2/Customizing-Git-Git-Attributes#Merge-Strategies But in the meantime, should this get re-labeled? I.e., drop If we do do that, we should similarly re-queue this one as well, which fixed a few minor things in follow-up: |
BZ: https://bugzilla.redhat.com/show_bug.cgi?id=1292961
This bug became something different from its original intent. It ended up covering a number of topics in the install guide, and detailed how certain things can be done with ansible, and providing example config.