Skip to content

OSDOCS#10176: Updates to Terraform files#77311

Merged
stevsmit merged 1 commit intoopenshift:mainfrom
EricPonvelle:OSDOCS-10177_Terraform-Updates
Jun 26, 2024
Merged

OSDOCS#10176: Updates to Terraform files#77311
stevsmit merged 1 commit intoopenshift:mainfrom
EricPonvelle:OSDOCS-10177_Terraform-Updates

Conversation

@EricPonvelle
Copy link
Contributor

@EricPonvelle EricPonvelle commented Jun 11, 2024

Version(s):
enterprise-4.15+

Issue:
OSDOCS-10176

Link to docs preview:

QE review:

  • QE has approved this change.

Additional information:
This PR updates the Terraform files for ROSA (classic architecture) and introduces Terraform for ROSA that uses HCP.

@openshift-ci openshift-ci bot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Jun 11, 2024
@EricPonvelle EricPonvelle force-pushed the OSDOCS-10177_Terraform-Updates branch 4 times, most recently from 7187fce to 60f8467 Compare June 12, 2024 19:10
@openshift-ci openshift-ci bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jun 12, 2024
@EricPonvelle EricPonvelle force-pushed the OSDOCS-10177_Terraform-Updates branch 5 times, most recently from ee27d9e to ef14f03 Compare June 13, 2024 21:14
@openshift-ci openshift-ci bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 13, 2024
@EricPonvelle EricPonvelle force-pushed the OSDOCS-10177_Terraform-Updates branch 2 times, most recently from cb239a2 to f8c1a35 Compare June 13, 2024 21:39
@EricPonvelle EricPonvelle force-pushed the OSDOCS-10177_Terraform-Updates branch from f8c1a35 to e3f9c62 Compare June 13, 2024 21:50
Copy link
Contributor

@jneczypor jneczypor left a comment

Choose a reason for hiding this comment

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

This is really good for such a large PR! Just a few tiny fixes. Most of my comments and questions revolve around the new naming system for ROSA (classic architecture) and ROSA. You may want to find out how often we are supposed to use parenthetical clarification of (classic architecture). Is it at every mention of ROSA? Just first mention? Will this change when our docs are separate? Now, Classic and HCP is mixed together so we may want to over-clarify a bit to ensure our users know they are on the correct product page. That will involve a lot of conditionalizing, especially in the modules where there is the most re-use. When we have separate docs, it may be less of an issue. Additionally, we will need to inform all our stakeholders and OCP review teams of this change and our guidelines for the name so that they can help us catch the ones we miss.

@EricPonvelle EricPonvelle force-pushed the OSDOCS-10177_Terraform-Updates branch 3 times, most recently from 114ecf4 to 35aefda Compare June 14, 2024 22:04
@EricPonvelle EricPonvelle force-pushed the OSDOCS-10177_Terraform-Updates branch from 84addea to c9a10a6 Compare June 24, 2024 16:57
@EricPonvelle EricPonvelle added the peer-review-needed Signifies that the peer review team needs to review this PR label Jun 24, 2024
@EricPonvelle EricPonvelle force-pushed the OSDOCS-10177_Terraform-Updates branch 2 times, most recently from f3c5d25 to 199ad4a Compare June 24, 2024 18:45
@JoeAldinger JoeAldinger added peer-review-in-progress Signifies that the peer review team is reviewing this PR and removed peer-review-needed Signifies that the peer review team needs to review this PR labels Jun 24, 2024
Copy link
Contributor

@JoeAldinger JoeAldinger left a comment

Choose a reason for hiding this comment

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

Some questions and a few suggestions.

- Name: Creating a ROSA cluster with STS using customizations
File: rosa-sts-creating-a-cluster-with-customizations
- Name: Creating a ROSA cluster with STS using Terraform
- Name: Creating a ROSA (classic architecture) cluster using Terraform
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure how the rule about parenthetical expressions applies to naming here, but I wanted to point it out.

Copy link
Member

Choose a reason for hiding this comment

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

There are examples in our docs of parentheticals being used in the topic map, usually smaller stuff like (STS) though. This is probably fine

#
# Copyright (c) 2023 Red Hat, Inc.
#
# Licensed under the Apache License, Version 2.0 (the "License");
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think I've ever seen this in the repo where the apache license information is given as part of a code sample. I doubled checked and couldn't find it like this anywhere else. Again, not saying this is wrong just pointing it out.

@JoeAldinger JoeAldinger added peer-review-done Signifies that the peer review team has reviewed this PR and removed peer-review-in-progress Signifies that the peer review team is reviewing this PR labels Jun 25, 2024
@EricPonvelle EricPonvelle force-pushed the OSDOCS-10177_Terraform-Updates branch from 199ad4a to 01c73e4 Compare June 25, 2024 19:24
@openshift-ci
Copy link

openshift-ci bot commented Jun 25, 2024

@EricPonvelle: all tests passed!

Full PR test history. Your PR dashboard.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

@EricPonvelle EricPonvelle added the merge-review-needed Signifies that the merge review team needs to review this PR label Jun 25, 2024
@ShaunaDiaz
Copy link
Contributor

@EricPonvelle I don't see QE or SME ack on this PR. Do you those?

@stevsmit stevsmit added merge-review-in-progress Signifies that the merge review team is reviewing this PR and removed merge-review-needed Signifies that the merge review team needs to review this PR labels Jun 26, 2024
@stevsmit stevsmit added this to the Continuous Release milestone Jun 26, 2024
- Name: Creating a ROSA cluster with STS using customizations
File: rosa-sts-creating-a-cluster-with-customizations
- Name: Creating a ROSA cluster with STS using Terraform
- Name: Creating a ROSA (classic architecture) cluster using Terraform
Copy link
Member

Choose a reason for hiding this comment

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

There are examples in our docs of parentheticals being used in the topic map, usually smaller stuff like (STS) though. This is probably fine

[id="rosa-classic-cluster-terraform-file-creation_{context}"]
= Creating your Terraform files locally

After you set up your link:https://console.redhat.com/openshift/token/rosa[offline {cluster-manager-first} token], you need to create the Terraform files locally to build your cluster. You can create these files by using the following code templates.
Copy link
Member

Choose a reason for hiding this comment

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

@stevsmit
Copy link
Member

@EricPonvelle Good work! Merging now. No glaring issues or anything like that.

@stevsmit stevsmit merged commit df38a71 into openshift:main Jun 26, 2024
@stevsmit
Copy link
Member

/cherry-pick enterprise-4.15

@stevsmit
Copy link
Member

/cherry-pick enterprise-4.16

@openshift-cherrypick-robot

@stevsmit: #77311 failed to apply on top of branch "enterprise-4.15":

Applying: OSDOCS#10177: Updates to Terraform files
.git/rebase-apply/patch:474: trailing whitespace.
    
.git/rebase-apply/patch:697: trailing whitespace.
  Enter a value: 
.git/rebase-apply/patch:703: trailing whitespace.
  Enter a value: 
.git/rebase-apply/patch:788: trailing whitespace.
  Enter a value: 
.git/rebase-apply/patch:793: trailing whitespace.
  Enter a value: 
warning: 5 lines add whitespace errors.
Using index info to reconstruct a base tree...
M	_topic_maps/_topic_map_rosa.yml
M	modules/rosa-sts-cluster-terraform-file-creation.adoc
M	modules/rosa-sts-overview-of-the-default-cluster-specifications.adoc
Falling back to patching base and 3-way merge...
Removing rosa_install_access_delete_clusters/terraform/rosa-sts-creating-a-cluster-quickly-terraform.adoc
Auto-merging modules/rosa-sts-overview-of-the-default-cluster-specifications.adoc
CONFLICT (content): Merge conflict in modules/rosa-sts-overview-of-the-default-cluster-specifications.adoc
CONFLICT (modify/delete): modules/rosa-sts-cluster-terraform-file-creation.adoc deleted in OSDOCS#10177: Updates to Terraform files and modified in HEAD. Version HEAD of modules/rosa-sts-cluster-terraform-file-creation.adoc left in tree.
Auto-merging _topic_maps/_topic_map_rosa.yml
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 OSDOCS#10177: Updates to Terraform files
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

Details

In response to this:

/cherry-pick enterprise-4.15

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@openshift-cherrypick-robot

@stevsmit: new pull request created: #78104

Details

In response to this:

/cherry-pick enterprise-4.16

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

Copy link

@radtriste radtriste left a comment

Choose a reason for hiding this comment

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

@EricPonvelle See my post merge review (sorry for the delay ...)

[discrete]
include::modules/rosa-sts-terraform-considerations.adoc[leveloffset=+1]

include::modules/rosa-sts-overview-of-the-default-cluster-specifications.adoc[leveloffset=+1]

Choose a reason for hiding this comment

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

Some cluster specifications has no sense in HCP, like:

  • Control plane node configuration component
  • Infrastructure plane node configuration component
  • Cluster settings -> Default EC2 IMDS endpoints
  • Encryption -> No KMS key encryption by default

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@radtriste Do I need to remove the first two items from the HCP flavors of the table? I created this PR that covers the items that I missed with your review: #78211

# limitations under the License.
#
module "vpc" {
source = "terraform-aws-modules/vpc/aws"

Choose a reason for hiding this comment

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

Do we want the customer to use the classic modules with BYOVPC as example ?
Because I would rather use rosa-classic vpc submodule to create the VPC

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@andyrepton should I update the VPC modules?

# limitations under the License.
#
module "vpc" {
source = "terraform-aws-modules/vpc/aws"

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@andyrepton should I update the VPC modules?

* All other AWS resources required to create a ROSA with STS cluster

include::modules/rosa-terraform-overview.adoc[leveloffset=+1]
include::modules/rosa-sts-terraform-prerequisites.adoc[leveloffset=+1]

Choose a reason for hiding this comment

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

This file contains See the Additional resources for more information on the AWS account roles. but I cannot find those additional resources

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@radtriste updated that mistake in this PR as well: #78211

@EricPonvelle EricPonvelle changed the title OSDOCS#10177: Updates to Terraform files OSDOCS#10176: Updates to Terraform files Jul 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

branch/enterprise-4.15 branch/enterprise-4.16 merge-review-in-progress Signifies that the merge review team is reviewing this PR peer-review-done Signifies that the peer review team has reviewed this PR size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.