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.

Sign up for GitHub

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

Jump to bottom

OSDOCS-2922: Added a warning for deleting clusters. #41128

Merged
merged 1 commit into from
Feb 3, 2022
Merged

OSDOCS-2922: Added a warning for deleting clusters. #41128

merged 1 commit into from
Feb 3, 2022
+19 −1

Conversation

EricPonvelle
Copy link
Contributor

@EricPonvelle EricPonvelle commented Jan 27, 2022 •
edited
Loading

@openshift-ci openshift-ci bot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Jan 27, 2022
@netlify
Copy link

netlify bot commented Jan 27, 2022 •
edited
Loading

✔️ Deploy Preview for osdocs ready!

🔨 Explore the source changes: 798f749

🔍 Inspect the deploy log: https://app.netlify.com/sites/osdocs/deploys/61fc40afb785f4000783142e

😎 Browse the preview: https://deploy-preview-41128--osdocs.netlify.app

@EricPonvelle EricPonvelle force-pushed the OSDOCS-2922_NetworkConfigNote branch from 8347e18 to a283d9d Compare January 28, 2022 15:03
@openshift-ci openshift-ci bot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jan 28, 2022
mjpytlak
mjpytlak reviewed Jan 28, 2022
View reviewed changes
modules/rosa-deleting-cluster.adoc Outdated

[IMPORTANT]
====
You must remove any network configurations, such as VPN configurations and VPC peering, before you delete a cluster. If these configurations remain, then the cluster will not delete properly.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
You must remove any network configurations, such as VPN configurations and VPC peering, before you delete a cluster. If these configurations remain, then the cluster will not delete properly.
You must remove any network configurations, such as VPN configurations and VPC peering, before you delete a cluster. If these configurations remain, then the cluster will not delete properly.
+
ifdef:sts[]
Account-wide IAM roles and policies might be used by other ROSA clusters in the same AWS account. You must only remove the resources if they are not required by other clusters.
endif::sts[]

@EricPonvelle
Copy link
Contributor Author

@okashi18, so I served this section three different ways. @mjpytlak made some good suggestions on how to show this correctly.

Currently, it will display two important notes on the STS side.
DeleteCluster-Current

This isn't really bad per se, since we want the user two note both important comments. These are separate warnings that can result in issues if ignored.

Another option that Mike documented in the ticket is to combine the two notes based on context. The first image shows the notes combined on STS:
DeleteCluster-Combined-STS

Then, the non-STS version:
DeleteCluster-Combined-non-STS

Finally, here's what it looks like if I change that second important note into a scarily worded paragraph:
DeleteCluster-In-Line

I am flexible with the way we should document this, though I think the second option is a bit too disrupting. I would prefer the third option, first option, then second. Mike can weigh in on this as well.

@mjpytlak
Copy link
Contributor

mjpytlak commented Jan 28, 2022 •
edited
Loading

@EricPonvelle As we discussed, I find that doc sets (not just Red Hat) overuse note and important admonitions. To take your 3rd option a step further (which I happen to like best), why not list removing network configurations (1st important) as a prerequisite?

Prerequisites

  • Remove any network configurations, such as VPN configurations and VPC peering before you delete a cluster. If these configurations remain, the cluster does not delete properly.

And to take this even a step further for STS:

Prerequisites

  • Remove any network configurations, such as VPN configurations and VPC peering before you delete a cluster. If these configurations remain, the cluster does not delete properly.
  • Account-wide IAM roles and policies might be used by other ROSA clusters in the same AWS account. Verify that the resources you are going to delete are not required by other clusters.

Note that I avoided the scary language all-together. :)

@0kashi
Copy link
Contributor

0kashi commented Jan 28, 2022 •
edited
Loading

@mjpytlak - I like that idea of putting the removal of network configurations in the prereqs. Though I would keep the note about the account roles.
@EricPonvelle - I do see those twice on the page. Do we even need the first one?

@EricPonvelle EricPonvelle force-pushed the OSDOCS-2922_NetworkConfigNote branch from a283d9d to d681e67 Compare January 31, 2022 16:33
@openshift-ci openshift-ci bot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jan 31, 2022
@EricPonvelle
Copy link
Contributor Author

@0kashi and @mjpytlak - thanks for the feedback! I updated the PR to include those changes. Basically, I added a prereq to both the STS and non-STS documentation, and kept the original Important note.

@EricPonvelle EricPonvelle force-pushed the OSDOCS-2922_NetworkConfigNote branch 2 times, most recently from 973ad96 to 2e16024 Compare February 2, 2022 21:42
@EricPonvelle EricPonvelle force-pushed the OSDOCS-2922_NetworkConfigNote branch 2 times, most recently from 5f671a4 to fa19c0d Compare February 2, 2022 22:37
@EricPonvelle EricPonvelle added this to the Next Release milestone Feb 2, 2022
@EricPonvelle EricPonvelle force-pushed the OSDOCS-2922_NetworkConfigNote branch from fa19c0d to 571aa3a Compare February 2, 2022 22:41
@0kashi
Copy link
Contributor

0kashi commented Feb 2, 2022

lgtm

@maxwelldb maxwelldb self-requested a review February 2, 2022 23:15
maxwelldb
maxwelldb reviewed Feb 2, 2022
View reviewed changes
Copy link
Contributor

@maxwelldb maxwelldb left a comment

Choose a reason for hiding this comment

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

Left some nits and feedback. Nothing major.

modules/rosa-deleting-cluster.adoc Show resolved Hide resolved
rosa_getting_started/rosa-deleting-cluster.adoc Outdated Show resolved Hide resolved
rosa_getting_started_sts/rosa-sts-deleting-cluster.adoc Outdated Show resolved Hide resolved
@maxwelldb maxwelldb added the peer-review-done Signifies that the peer review team has reviewed this PR label Feb 2, 2022
@EricPonvelle EricPonvelle force-pushed the OSDOCS-2922_NetworkConfigNote branch 4 times, most recently from 55539b0 to cde9b3d Compare February 3, 2022 19:20
maxwelldb
maxwelldb reviewed Feb 3, 2022
View reviewed changes
rosa_getting_started/rosa-deleting-cluster.adoc Outdated
== Prerequisites

* If {product-title} created a VPC, you must remove the following before you delete your cluster:
* network configurations, such as VPN configurations and VPC peering
Copy link
Contributor

Choose a reason for hiding this comment

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

Should "VPC peering" be something like "VPC peering connections" or "VPC peering configurations"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@0kashi - which one should it be?

Copy link
Contributor

Choose a reason for hiding this comment

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

VPC peering connections is fine

maxwelldb
maxwelldb reviewed Feb 3, 2022
View reviewed changes
rosa_getting_started/rosa-deleting-cluster.adoc Outdated
[id="prerequisites_rosa-deleting-cluster"]
== Prerequisites

* If {product-title} created a VPC, you must remove the following before you delete your cluster:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* If {product-title} created a VPC, you must remove the following before you delete your cluster:
* If {product-title} created a VPC, you must remove the following [$PLURAL_NOUN] before you delete your cluster:

Better for translation, but nothing in particular is coming to mind. :(

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@maxwelldb This work?

  • If {product-title} created a VPC, you must remove the following items from your cluster before you can successfully delete your cluster:

@EricPonvelle EricPonvelle force-pushed the OSDOCS-2922_NetworkConfigNote branch 4 times, most recently from c5fa592 to 269af2d Compare February 3, 2022 20:34
@EricPonvelle EricPonvelle force-pushed the OSDOCS-2922_NetworkConfigNote branch from 269af2d to 798f749 Compare February 3, 2022 20:53
@EricPonvelle EricPonvelle merged commit e0a1437 into openshift:main Feb 3, 2022
@EricPonvelle
Copy link
Contributor Author

/cherry-pick enterprise-4.9

@EricPonvelle
Copy link
Contributor Author

/cherry-pick enterprise-4.10

@openshift-cherrypick-robot openshift-cherrypick-robot mentioned this pull request Feb 3, 2022
Merged
@openshift-cherrypick-robot
Copy link

@EricPonvelle: new pull request created: #41389

In response to this:

/cherry-pick enterprise-4.9

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/test-infra repository.

@openshift-cherrypick-robot openshift-cherrypick-robot mentioned this pull request Feb 3, 2022
Merged
@openshift-cherrypick-robot
Copy link

@EricPonvelle: new pull request created: #41390

In response to this:

/cherry-pick enterprise-4.10

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/test-infra repository.

@EricPonvelle EricPonvelle deleted the OSDOCS-2922_NetworkConfigNote branch February 4, 2022 14:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mjpytlak mjpytlak mjpytlak left review comments

@0kashi 0kashi 0kashi left review comments

@maxwelldb maxwelldb maxwelldb left review comments

Assignees
No one assigned
Labels
branch/enterprise-4.9 branch/enterprise-4.10 peer-review-done Signifies that the peer review team has reviewed this PR size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Milestone
Next Release
Development

Successfully merging this pull request may close these issues.
5 participants
@EricPonvelle @mjpytlak @0kashi @openshift-cherrypick-robot @maxwelldb