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

OSSMDOC-259: OSD changes. Peer review done, waiting for coordination with other teams. #30066

Merged
merged 1 commit into from
Jun 24, 2021

Conversation

neal-timpe
Copy link
Contributor

@neal-timpe neal-timpe commented Mar 3, 2021

  • Aligned team: Service mesh
  • For branches: 4.6, 4.7, 4.8
  • https://issues.redhat.com/browse/OSSMDOC-259
  • At each mention of cluster-admin, adds a sentence that says if you're using dedicated, the role is dedicated admin.
  • This work makes changes so we can use this content for OSD also.
  • SME review: knrc
  • QE review: tvieira, gbaufake
  • Peer review: JStickler, mikemckiernan

@openshift-ci-robot openshift-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Mar 3, 2021
@netlify
Copy link

netlify bot commented Mar 3, 2021

✔️ Deploy Preview for osdocs ready!

🔨 Explore the source changes: e5d36ee

🔍 Inspect the deploy log: https://app.netlify.com/sites/osdocs/deploys/60d4f16db317fe00082cb3ea

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

@knrc
Copy link

knrc commented Mar 4, 2021

@neal-timpe Was it not possible to go with a generic admin term and then define that to be cluster-admin/dedicated-admin in a single location? For example

Throughout this documentation you will have occasion to run commands as an admin user. If you are running in an OpenShift Dedicated cluster this would require the use of an account with the dedicated-admin role, otherwise it would require the use of an account with the cluster-admin role.

Then we could just refer to an admin account in the body of the docs.

@neal-timpe
Copy link
Contributor Author

@knrc Heather and I considered that approach, but decided that the way the our content is structured that it would be better to do it this way.

@knrc
Copy link

knrc commented Mar 4, 2021

@neal-timpe okay thanks, in that case lgtm

Copy link

@tvieira tvieira left a comment

Choose a reason for hiding this comment

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

LGTM

@neal-timpe neal-timpe changed the title OSSMDOC-259 OSSMDOC-259 - Needs review, but not ready to merge yet Mar 8, 2021
@neal-timpe neal-timpe changed the title OSSMDOC-259 - Needs review, but not ready to merge yet OSSMDOC-259 - Needs peer review, but not ready to merge yet Mar 8, 2021
Copy link
Contributor

@JStickler JStickler left a comment

Choose a reason for hiding this comment

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

The installing-ossm assembly pulls in jaeger-install-elasticsearch and jaeger-install, which haven't been updated by this PR.

@neal-timpe neal-timpe added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Mar 10, 2021
@neal-timpe neal-timpe changed the title OSSMDOC-259 - Needs peer review, but not ready to merge yet OSSMDOC-259 - Peer review done, waiting for coordination with other teams. Mar 10, 2021
@neal-timpe neal-timpe force-pushed the ossmdoc-259 branch 2 times, most recently from 6068182 to 5c829ff Compare April 12, 2021 22:14
@neal-timpe neal-timpe changed the title OSSMDOC-259 - Peer review done, waiting for coordination with other teams. OSSMDOC-259: OSD changes. Peer review done, waiting for coordination with other teams. Jun 2, 2021
@@ -106,7 +106,7 @@ A {ProductName} control plane component called Istio OpenShift Routing (IOR) syn

[id="ossm-catch-all-domains_{context}"]
=== Catch-all domains
Catch-all domains ("\*") are not supported. If one is found in the Gateway definition, {ProductName} _will_ create the route, but will rely on OpenShift to create a default hostname. This means that the newly created route will __not__ be a catch all ("*") route, instead it will have a hostname in the form `<route-name>[-<project>].<suffix>`. Refer to the OpenShift documentation for more information about how default hostnames work and how a cluster administrator can customize it.
Catch-all domains ("\*") are not supported. If one is found in the Gateway definition, {ProductName} _will_ create the route, but will rely on OpenShift to create a default hostname. This means that the newly created route will __not__ be a catch all ("*") route, instead it will have a hostname in the form `<route-name>[-<project>].<suffix>`. Refer to the OpenShift documentation for more information about how default hostnames work and how a `cluster-admin` can customize it. If you are using {product-dedicated}, refer to the {product-dedicated} the `dedicated-admin` role.
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 expect you to remove the future tense and markup from the para, but I sure hope someone does.

The "OpenShift" alone in that para is making my other eye twitch. That should probably be an attribute that expands to "OpenShift Container Platform."

Copy link
Contributor

@mikemckiernan mikemckiernan left a comment

Choose a reason for hiding this comment

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

I have one sugg to sidestep "using." If you agree with it, seems like it gets applied throughout.

@openshift-ci openshift-ci bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 24, 2021
@neal-timpe neal-timpe force-pushed the ossmdoc-259 branch 2 times, most recently from cbc4836 to 0bf4022 Compare June 24, 2021 20:12
@openshift-ci openshift-ci bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 24, 2021
@mikemckiernan mikemckiernan merged commit 290e86b into openshift:master Jun 24, 2021
@mikemckiernan
Copy link
Contributor

/cherrypick enterprise-4.8

@openshift-cherrypick-robot

@mikemckiernan: new pull request created: #33950

In response to this:

/cherrypick enterprise-4.8

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.

@mikemckiernan
Copy link
Contributor

/cherrypick enterprise-4.7

@openshift-cherrypick-robot

@mikemckiernan: new pull request created: #33951

In response to this:

/cherrypick enterprise-4.7

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.

@mikemckiernan
Copy link
Contributor

/cherrypick enterprise-4.6

@openshift-cherrypick-robot

@mikemckiernan: #30066 failed to apply on top of branch "enterprise-4.6":

Applying: add dedicatedadmin
Using index info to reconstruct a base tree...
M	modules/jaeger-install-elasticsearch.adoc
M	modules/jaeger-install.adoc
M	modules/ossm-supported-configurations.adoc
Falling back to patching base and 3-way merge...
Auto-merging modules/ossm-supported-configurations.adoc
Auto-merging modules/jaeger-install.adoc
CONFLICT (content): Merge conflict in modules/jaeger-install.adoc
Auto-merging modules/jaeger-install-elasticsearch.adoc
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 add dedicatedadmin
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".

In response to this:

/cherrypick enterprise-4.6

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
branch/enterprise-4.6 branch/enterprise-4.7 branch/enterprise-4.8 service-mesh Label for all Service Mesh PRs size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants