Skip to content

Conversation

@gaurav-nelson
Copy link
Contributor

@gaurav-nelson gaurav-nelson commented Dec 11, 2017

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1520090

  • Removed the file admin_guide/aws_cluster_labeling.adoc
  • Added the contents of deleted files in install_config/configuring_aws.adoc
  • Updated _topic_map.yml (removed entry for deleted file)
  • Checked for existing links to the deleted file and updated the single link found in 3.7 release notes.

@openshift-ci-robot openshift-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Dec 11, 2017
@gaurav-nelson gaurav-nelson added this to the Next Release milestone Dec 11, 2017
@gnunn1
Copy link

gnunn1 commented Dec 11, 2017

Can openshift_clusterid be documented here as well?

@gaurav-nelson
Copy link
Contributor Author

@gnunn1 sure, I'll add that.

@gaurav-nelson gaurav-nelson changed the title BZ#1520090 Fixes - Updated AWS Configuration to include cluster labeling Bug#1520090 - Updated AWS Configuration to include cluster labeling Dec 13, 2017
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@gnunn1 I have added this note which includes clusterid info. Does this looks alright?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am also trying to get more info about openshift_clusterid variable meanwhile.

@gaurav-nelson
Copy link
Contributor Author

@gnunn1
Copy link

gnunn1 commented Dec 20, 2017

Looks good but you are still meaning documenting the openshift_clusterid variable in the Configuring {product-title} for AWS with Ansible. This variable is mentioned in the 3.7 release notes under the section Labeling Clusters for Amazon Web Services:

Labeling Clusters for Amazon Web Services
Starting with 3.7 versions of the installer, if you configured AWS provider credentials, you must also ensure that all instances are labeled. Then, set the openshift_clusterid variable to the cluster ID. See Labeling Clusters for Amazon Web Services (AWS) for more information.

https://docs.openshift.com/container-platform/3.7/release_notes/ocp_3_7_release_notes.html#ocp-37-notable-technical-changes

@vikram-redhat vikram-redhat modified the milestones: Next Release, Staging Jan 8, 2018
@gaurav-nelson
Copy link
Contributor Author

@gnunn1 PTAL
I have:

@gaurav-nelson
Copy link
Contributor Author

@gnunn1 bump!

@gnunn1
Copy link

gnunn1 commented Jan 10, 2018

@gaurav-nelson I don't believe that the variable openshift_clusterid should be in the Setting Ansible Variables, it should be in the section Configuring {product-title} for AWS with Ansible with the other variables. Was Setting Ansible Variables always there, if not just remove it and put openshift_clusterid in the other section with the other ansible variables.

Otherwise it looks fine to me.

@gaurav-nelson
Copy link
Contributor Author

gaurav-nelson commented Jan 11, 2018

Thanks @gnunn1 PTAL(Preview:https://github.com/gaurav-nelson/openshift-docs/blob/4983d3b56eb27186c48444623f5824ac13fc4c8f/install_config/configuring_aws.adoc#aws-configuring-masters)
Latest commit fixes these nits:

  • Removed the Setting Ansible Variables section
  • Added openshift_clusterid variable to Configuring {product-title} for AWS with Ansible section

@gaurav-nelson
Copy link
Contributor Author

@gnunn1 PTAL

@gnunn1
Copy link

gnunn1 commented Jan 15, 2018

Just add openshift_clusterid in the example with the other variables and I am fine.

@gaurav-nelson
Copy link
Contributor Author

gaurav-nelson commented Jan 15, 2018

Thanks @gnunn1 I have included it in the list with other variables.

@gaurav-nelson gaurav-nelson added the peer-review-needed Signifies that the peer review team needs to review this PR label Jan 15, 2018
@gaurav-nelson
Copy link
Contributor Author

@openshift/team-documentation PTAL

Choose a reason for hiding this comment

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

What does "for the installation program" refer to? I don't think this is an OpenShift thing. Would it be easier to take it out?

Choose a reason for hiding this comment

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

I can see this section was like this already, but i'm wondering if it can be merged into the 'Tagging an Existing Cluster' section below. This isn't adding much that the section below isn't already saying. WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @bfallonf I am keeping this as it is for now because I think the next section is a procedure and this section is just calling what needs to be tagged. You are right that both of these sections should be combined, I will revisit this later.

@bfallonf
Copy link

@gaurav-nelson A few comments, but looks good overall.

@bfallonf bfallonf added peer-review-done Signifies that the peer review team has reviewed this PR and removed peer-review-needed Signifies that the peer review team needs to review this PR labels Jan 16, 2018
@gaurav-nelson gaurav-nelson merged commit 2c85603 into openshift:master Jan 18, 2018
@gaurav-nelson gaurav-nelson deleted the bug1520090-fixes branch January 18, 2018 03:00
@gaurav-nelson
Copy link
Contributor Author

moved existing content around, no revision history needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

branch/enterprise-3.7 branch/enterprise-3.9 peer-review-done Signifies that the peer review team has reviewed this PR size/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants