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

Some docs updates for the 0.2.0 release #12233

Merged
merged 2 commits into from
Oct 17, 2018
Merged

Conversation

knrc
Copy link

@knrc knrc commented Sep 28, 2018

No description provided.

@openshift-ci-robot openshift-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Sep 28, 2018
@kalexand-rh
Copy link
Contributor

@JStickler, PTAL

Copy link
Contributor

@kalexand-rh kalexand-rh left a comment

Choose a reason for hiding this comment

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

Couple of style picks. Unless you're referring to a specific OpenShift element, you can't just say "OpenShift." You need to use the {product-title} variable to apply the correct OpenShift product name for the collection.

** Adds the cluster admin role to the OpenShift user specified in the launcher parameters in the custom resource file.
Copy link
Contributor

Choose a reason for hiding this comment

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

s/OpenShift/{product-title} (multiple places, multiple files)

Copy link
Author

Choose a reason for hiding this comment

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

This was not actually a line I had intended to change, looks as if it was just a newline appended to the content. I can make the change however.

Copy link
Author

Choose a reason for hiding this comment

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

This should now be done

|`prefix`
|Any valid image repo.
|Which prefix to apply to the Kiali image name used in docker pull.
|If deployment_type=origin the default is `kiali/`.
Copy link
Contributor

Choose a reason for hiding this comment

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

s/deployment_type=origin/`deployment_type=origin` (two places)

Copy link
Author

Choose a reason for hiding this comment

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

This should now be done

|Which prefix to apply to the Kiali image name used in docker pull.
|If deployment_type=origin the default is `kiali/`.

If deployment_type=openshift the default is `openshift-istio-tech-preview/`.
Copy link
Contributor

Choose a reason for hiding this comment

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

s/deployment_type=openshift/`deployment_type=openshift`, (two places)

Copy link
Author

Choose a reason for hiding this comment

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

This should now be done

If deployment_type=openshift then `0.7.2`.

|`username`
|The username used to access the Kiali console. Note that this is not related to any account on OpenShift
Copy link
Contributor

Choose a reason for hiding this comment

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

s/username/user name

Copy link
Author

Choose a reason for hiding this comment

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

This should now be done

@@ -10,12 +10,18 @@ The {ProductShortName} installation process introduces a Kubernetes operator to
== Installing the operator
The following steps will install the {ProductShortName} operator into an existing {product-title} 3.10 installation; they can be executed from any host with access to the cluster. Please ensure you are logged in as a cluster admin before executing the following commands.

You can find the https://github.com/Maistra/openshift-ansible/tree/maistra-0.1.0-ocp-3.1.0-istio-1.0.0/istio[operator templates on GitHub].
You can find the https://github.com/Maistra/openshift-ansible/tree/maistra-0.2.0-ocp-3.1.0-istio-1.0.2/istio[operator templates on GitHub].
Copy link
Contributor

Choose a reason for hiding this comment

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

@vikram-redhat, are you ok with this GitHub reference?

Copy link
Contributor

Choose a reason for hiding this comment

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

Hi Kevin - @knrc - is this the upstream for ServiceMesh?

Copy link
Author

Choose a reason for hiding this comment

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

@vikram-redhat Yes it is.

Copy link
Contributor

Choose a reason for hiding this comment

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

@knrc - is there a RH supported link instead? If not, is it confirmed within these docs that Service Mesh is not supported by RH and that this is dev preview only? If yes, we can approve this link.

Copy link
Author

Choose a reason for hiding this comment

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

@vikram-redhat There is no product/supported link as of yet and this release is described as a tech preview within the docs and the images.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks Kevin (@knrc)!

@kalexand-rh - yes we can accept this link.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks Vikram! When Kevin clears the merge errors, I'll merge this PR. :)


```
$ oc new-project istio-operator
$ oc new-app -f istio_product_operator_template.yaml --param=OPENSHIFT_ISTIO_MASTER_PUBLIC_URL=<master public url>
```
[NOTE]
.OpenShift Master Public URL
=====================
Copy link
Contributor

Choose a reason for hiding this comment

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

Four "=" signs

Remove the note label.

Rewrite to not use "should."

s/your OpenShift Console, this/{product-title}. This

Copy link
Author

Choose a reason for hiding this comment

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

This should now be done

@@ -236,22 +267,6 @@ $ oc ex config patch master-config.yaml.prepatch -p "$(cat master-config.patch)"
$ master-restart api
$ master-restart controllers
```
+
. Then you must modify each individual deployment that you want to monitor as part of your service mesh to enable automatic sidecar injection. Each deployment where you want to enable automatic injection needs to contain the `sidecar.istio.io/inject: "true":` annotation.
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 necessarily think this belongs here, but this is the only place on the OSSM docs that mentions the annotation. I don't think we can entirely remove this.

Copy link
Author

Choose a reason for hiding this comment

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

This is more to do with how to deploy applications and take advantage of the automatic injection so has nothing to do with installation. It definitely shouldn't be in this section, perhaps we need to introduce a new one covering how to develop applications using automatic injection?

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree. A separate section would be great. Just want to make sure this makes it into the doc.

@openshift-bot openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 14, 2018
@kalexand-rh
Copy link
Contributor

@JStickler, @knrc, does this content still need to be merged?

@knrc
Copy link
Author

knrc commented Oct 17, 2018

@knrc Yes, I made all the requested changes and since then it has been sitting. We are releasing 0.3.0 next week so there will be another PR issued later this week to update to that release.

@knrc
Copy link
Author

knrc commented Oct 17, 2018

@kalexand-rh Sorry, meant to tag you and not myself :)

@kalexand-rh
Copy link
Contributor

@knrc, will you rebase?

@knrc
Copy link
Author

knrc commented Oct 17, 2018

@kalexand-rh Will it then be merged?

@kalexand-rh
Copy link
Contributor

@knrc, yes, I'll merge it. (@vikram-redhat, FYI)

@knrc
Copy link
Author

knrc commented Oct 17, 2018

@kalexand-rh Great I'll get the rebase done within the hour in that case, just finishing off another task. Thanks very much.

@kalexand-rh
Copy link
Contributor

Thanks @knrc! I'll check back after lunch and see if it's ready to go. 😸

@knrc
Copy link
Author

knrc commented Oct 17, 2018

@kalexand-rh There are now quite a few clashes so it looks as if I'll need to start afresh, rebase is throwing up too many differences. I'll get this done later today.

@kalexand-rh
Copy link
Contributor

@knrc, fair enough. Tag me in the new PR.

@knrc
Copy link
Author

knrc commented Oct 17, 2018 via email

@vikram-redhat
Copy link
Contributor

@knrc looks like the rebase is required due to the changes introduced by this PR: #12198

@kalexand-rh
Copy link
Contributor

@vikram-redhat, Kevin said that he'd fix up the PR soon.

@openshift-bot openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 17, 2018
@openshift-ci-robot openshift-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 17, 2018
@knrc
Copy link
Author

knrc commented Oct 17, 2018

@kalexand-rh @vikram-redhat I've been through all the changes and most are already applied to the docs, I've updated this PR to cover the remaining changes.

@kalexand-rh
Copy link
Contributor

kalexand-rh commented Oct 17, 2018

@knrc, there's a build error:

ERROR: servicemesh-install.adoc: line 75: include file not found:
/home/travis/build/openshift/openshift-docs/drupal-build/openshift-enterprise/servicemesh-install/topics/revision-info.adoc

Will you PTAL?

@knrc
Copy link
Author

knrc commented Oct 17, 2018

@kalexand-rh I don't understand the error, it's not from a file I modified

servicemesh-install/topics/install-overview.adoc
servicemesh-install/topics/install-prerequisites.adoc
servicemesh-install/topics/install.adoc

@knrc
Copy link
Author

knrc commented Oct 17, 2018

@kalexand-rh I can find no evidence that file was ever committed to the repository, I'm guessing someone forgot to add it.

@knrc
Copy link
Author

knrc commented Oct 17, 2018

@JStickler Any ideas?

@JStickler
Copy link
Contributor

@knrc The servicemesh-install.adoc file does have a commented out reference to the servicemesh-install.adoc that was there in the templates. Since I'm not using the revisions file, I never checked it in. But that hasn't thrown any errors until now. The only thing I can think of is if your text editor is configured to remove blank spaces at the end of files? Or if the rebase somehow mangled that include being commented out?

@knrc
Copy link
Author

knrc commented Oct 17, 2018

@JStickler I gave up on the rebase in the end, too many differences. I went through each change one by one and reapplied them to the appropriate location in the docs. This file is not one I modified hence why I'm asking for help :)

@knrc
Copy link
Author

knrc commented Oct 17, 2018

@JStickler @kalexand-rh @vikram-redhat Looking at the file the contents have not changed in over a month, and still appear to be commented out, so is this an error with the tooling?

@vikram-redhat
Copy link
Contributor

@knrc looking..

@vikram-redhat
Copy link
Contributor

@knrc @JStickler - yes, it looks like the build is still trying to include the revision history file even though it is part of a commented out block. I do recall this issue coming up about 4 months ago. What is mystifying though, is that it didn't come up before now.

It is unlikely to get fixed (as the eng team responsible for these tools were part of the recent changes), so to get around it, @knrc, could you please comment it out explicitly by adding a // in front of it, or just simply delete that line and add that file as part of this PR?

@knrc
Copy link
Author

knrc commented Oct 17, 2018

@vikram-redhat done

@vikram-redhat
Copy link
Contributor

@knrc - thanks and the build passes. Cool. I will merge as this was already peer reviewed.

@vikram-redhat vikram-redhat added the peer-review-done Signifies that the peer review team has reviewed this PR label Oct 17, 2018
@vikram-redhat vikram-redhat merged commit 6e174f1 into openshift:master Oct 17, 2018
@vikram-redhat
Copy link
Contributor

/cherrypick enterprise-3.10

@openshift-cherrypick-robot

@vikram-redhat: new pull request created: #12542

In response to this:

/cherrypick enterprise-3.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.

@vikram-redhat
Copy link
Contributor

/cherrypick enterprise-3.11

@openshift-cherrypick-robot

@vikram-redhat: new pull request created: #12543

In response to this:

/cherrypick enterprise-3.11

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-3.10 branch/enterprise-3.11 peer-review-done Signifies that the peer review team has reviewed this PR 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