Peer review - Githubissues

Pre-everything

[x] page-tags: contain {kube} which will not render because the variable kube is defined later in the guide
[x] 30 mins seems too long for this guide.
[x] minikube-ip variable is never used in the adoc.
[x] Capitalize "istio" in page-tags and add istioctl since kubectl is also there.

General/other

[x] compiling the adoc results in

asciidoctor: WARNING: README.adoc: line 265: tag 'copyright' not found in include file: /Users/dima/Documents/repos/guides/wip/draft-guide-istio/finish/kubernetes.yaml
asciidoctor: WARNING: README.adoc: line 308: tag 'copyright' not found in include file: /Users/dima/Documents/repos/guides/wip/draft-guide-istio/finish/routing.yaml

You need to add a copyright statement + tag into these files.

[x] you need to add more grammatical "sugar" to your sentences. I understand that you want to be concise, but some of your sentences feel bare bones.
[x] Always write inventory and system in code form like this -> inventory and system.
[x] Avoid using "we".

What you’ll learn

[x] "kubernetes" should be capitalized
[x] mention what the system service does in the second paragraph as you did with the inventory service

What is Istio?

[x] "Istio is a platform that provides several features that allow you manage various aspects of how your microservices interact with eachother and the outside world." -> change to -> "Istio is a platform for managing how microservices interact with each other as well as the outside world"
[x] whats an "Envoy proxy"? whenever you use unfamiliar concepts, try to briefly explain what they do. You can provide some links for more educational material later in the guide, but make sure they dont break the flow of reading; ie. the user shouldn't have to pause and go to another page to read up of stuff.
[x] give examples of what features Istio uses

Why use Istio?

[x] can you provide an example here? At this point, a reader should know enough about Istio to care about using it. In this section, I would take about one or two secnarios that could be solved with Istio and integrate nicely with it. Otherwise, the topic of Istio is stil too vague as this point for the user to care.

Prerequisites

[x] provide links to installation instructions for everything listed here not just istioctl
[x] mention that Docker is also necessary like I do in Kuberentes

Starting and preparing your cluster for Deployment

[x] this section feels incomplete, you just list a bunch of commands without much context as to why they need to be run or what they accomplish. For example, say something like "To begin working with your cluster, first start it by using Minikube" rather than "Start Minikube with these flags to use Istio". The users might not know what minikube is since you didn't explain it.
[ ] can you combine windows and unix commands into a single listing block? You can add comments about the commands to specify what platform they are meant for
[x] "As a result of this command, if you issue docker commands ..." -> explain why running this command is necessary for this guide; ie. because Docker images need to be built directly to Minikube.
[x] explain why the istio-system namespace should be created. Is it for organizational purposes? If so, make sure that state that.
[x] what does "deploying istio to your cluster" do? Explain what the kubectl apply command does, as well as what istio.yaml is and what it creates.
[x] "If you observe any errors, run the command one more time and it should succeed without any errors." -> this is kind of weird. Does the command fail often on its first try? Is there a reason why? Is there a workout/way to avoid this problem?
[x] Explain what kubectl get deployments does and what exactly the user should be looking for after they run that command.
[x] I couldn't start a Minikube cluster on my Mac with the command you provided. I ended up running the kube 1.10 command from here to start my cluster: https://istio.io/docs/setup/kubernetes/quick-start/#download-and-prepare-for-the-installation
[x] "path/to/istio/install/kubernetes/helm/istio" -> be more specific that this path is where Istio is downloaded.

Deploying version 1 of the services to cluster

[x] Explain what mvn clean package does; ie. "Run the mvn clean package command to build your microservice"
[x] Explain how mvn clean package builds a Docker image; ie. that it uses the dockerfile-maven profile to build a Docker image as part of the Maven package phase.
[x] Separate docker images into its own listing block so its easier to see and copy.
[x] "To deploy the services" -> Be specific what services because Kubernetes has its own notion of service. Specify that you're deploying the inventory and system microservice that are packaged into their respective Docker images.
[x] Explain what the kubernetes.yaml is for. For example, "The kubernetes.yaml configuration file contains configuration for various Kuberntes objects. In this case, it contains... , which can be broken down as so: ".
[x] "To get the base url, use the following command:" -> this command seems too complicated. Is there are better alternative? You need the cluster ip and port # correct? The cluster ip can be obtained via minikube ip and the port # via the port thats mapped under your service, which you can see after running kubectl get service. - [ ] Also, does this command work on Windows?
[x] Change http://ip-address:port to http://<cluster-ip>:<nodePort> for it to be more obvious that its a generalized URL, and that its user-specific.
[x] Separate kubectl get deployments into its own listing block so its easier to see and copy.

Modify and deploy version 2 of the service

[x] Give a brief context of what the user will do in this section and what will that ultimately achieve; ie. they will modify a file and deploy it as a new version, and then route between the old and new version via Istio's Ingress.
[x] You might have to revisit this section later when the model changes go in.
[x] "This will introduce" -> be specific. "This" as in "Renaming the getTotal() method to getCount()"
[x] "Before version 2 is deployed:" -> "Before version 2 is deployed, the /inventory/systems endpoint responds with a JSON in the following form"
[x] "The reason behind running both services is so that old clients do not have to upgrade to use the new api, they can simply continue to use the old one." -> add more sugar.
[x] Its kind of hard to find exactly what was added to the kubernetes.yaml. Can you mention the name of the object(s) as well?
[x] Users might not have curl available. Can they just visit that URL in a regular browser tab?
[x] "Observe that requests go to both v1 and v2." -> The user must open multiple sessions in order to see this. Make sure to mention that. You could ask the users to open unique sessions to see this as well (via curl or whatever).
[x] Also, explain why requests go to both versions. I guess this is because of load balancing done by the Ingress?

Configure request routing

[x] Give some contexts to what the user will do in this section, as well as how they will do it.
[x] "header" as in "HTTP header"? Be specific. Also explain what the header is for. Is to hold additional information when making HTTP requests?
[x] "Apply your routing rules by running the given command." -> "Apply the routing rules you created in the routing.yaml file by running the given command."
[x] Again, is there an alternative to curling?
[x] Also, split the curl commands into their own listing blocks so they are easier to see and copy.
[x] Add a small paragraph explaining what routing ultimately achieves. Perhaps something similar as in "Why use Istio" or in "What you'll learn".

Testing microservices that are running on Kubernetes

[x] Give some context to why running tests is important. See other guides for this. They all have a paragraph explaining why integration tests are important when developing.
[x] Remove the block comments from the test listing block. You can do this by surrounding the comments in tags and then excluding those tags in the adoc.
[x] Break down each test case that you added.
[x] Why repeat the tests three times? Does routing not always work? Is routing inconsistent? Thats really weird if it is.
[x] Explain what each flag is for in the test command.

[x] Show the expected test output. For example:


-------------------------------------------------------
T E S T S
-------------------------------------------------------
Running it.io.openliberty.guides.inventory.InventoryVersionTest
Tests run: 3, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 2.98 sec - [ ] in it.io.openliberty.guides.inventory.InventoryVersionTest

Results :

Tests run: 3, Failures: 0, Errors: 0, Skipped: 0


- [x] `inventory/pom.xml` contains a profile called `setup-tests` which starts the system service. This was probably left in from another guide.

**Conclusions**

- [x] add a section after the tests that explains how to teardown everything: chart release, minikube, etc.

**Great work! You’re done!**

- [x] typo: "to" -> "two"
- [x] "You have deployed to services to Kubernetes and routed requests to different versions of the inventory service based on http headers." -> "You have just deployed two microservice to a Kubernetes clutster and created routing rules to route between two different version of one of the microservice using Istio." or something like that.

@Kubik42 I updated the guide based on your feedback and checked off the boxes that I completed, but I have some questions/concerns about some of the feedback.

can you combine windows and unix commands into a single listing block? You can add comments about the commands to specify what platform they are meant for

@evelinec was against this... can you verify how you want this to be displayed please @evelinec ?

If you observe any errors, run the command one more time and it should succeed without any errors." -> this is kind of weird. Does the command fail often on its first try? Is there a reason why? Is there a workout/way to avoid this problem?

~This issue happens roughly 20% of the time I'd say.~ No longer an issue, I'll remove this from the guide.

Separate docker images into its own listing block so its easier to see and copy. Separate kubectl get deployments into its own listing block so its easier to see and copy.

Why would the user want to copy command output?

Users might not have curl available. Can they just visit that URL in a regular browser tab?

No, but they could use postman instead: https://www.getpostman.com/

Remove the block comments from the test listing block. You can do this by surrounding the comments in tags and then excluding those tags in the adoc.

I don't really understand, can you elaborate please?

Break down each test case that you added.

Don't I already do this?

Why repeat the tests three times? Does routing not always work? Is routing inconsistent? Thats really weird if it is.

I explained it, is it unclear?

Why would the user want to copy command output?

you have mark command output with role="no_copy" to make them uncopiable

No, but they could use postman instead: https://www.getpostman.com/

I don't really understand, can you elaborate please?

in you listing block, you can see the comments above the methods:

You can remove these by adding tags to your file and then removing those tags in your include statement in your adoc.

Don't I already do this?

I meant make it more clear. For example, "The lol() test case verifies..." for each test case that you have.

I explained it, is it unclear?

I know that you did, but it seems weird to me that this is necessary. Are you saying the routing rules may not always work? If you're demonstrating how something is done, it should work, so there is not need to rerun the test multiple times.

@evelinec Can you confirm if you want the unix/windows blocks split up, or if you want them together with a comment to distinguish them?

Just last few bits:

there are some differences in the POMs, is this intentional?

diff -ra finish/inventory/pom.xml start/inventory/pom.xml
8c8
<         <version>2.0-SNAPSHOT</version>
---
>         <version>1.0-SNAPSHOT</version>
54a55,125
>         <!-- Start system service before running tests -->
>         <profile>
>             <id>setup-test</id>
>             <activation>
>                 <activeByDefault>true</activeByDefault>
>             </activation>
>             <build>
>                 <plugins>
>                     <plugin>
>                         <groupId>org.codehaus.mojo</groupId>
>                         <artifactId>exec-maven-plugin</artifactId>
>                         <version>${version.exec-maven-plugin}</version>
>                         <executions>
>                             <execution>
>                                 <id>test-start-system-service</id>
>                                 <goals>
>                                     <goal>exec</goal>
>                                 </goals>
>                                 <phase>pre-integration-test</phase>
>                                 <configuration>
>                                     <workingDirectory>../system</workingDirectory>
>                                     <executable>mvn${mvn.extension}</executable>
>                                     <arguments>
>                                         <argument>liberty:start-server</argument>
>                                     </arguments>
>                                 </configuration>
>                             </execution>
>                         </executions>
>                     </plugin>
>
>                     <!-- Build docker image -->
>                     <plugin>
>                         <groupId>com.spotify</groupId>
>                         <artifactId>dockerfile-maven-plugin</artifactId>
>                     </plugin>
>                 </plugins>
>             </build>
>         </profile>
>
>         <!-- Stop system service after running tests -->
>         <profile>
>             <id>teardown-test</id>
>             <activation>
>                 <activeByDefault>true</activeByDefault>
>             </activation>
>             <build>
>                 <plugins>
>                     <plugin>
>                         <groupId>org.codehaus.mojo</groupId>
>                         <artifactId>exec-maven-plugin</artifactId>
>                         <version>${version.exec-maven-plugin}</version>
>                         <executions>
>                             <execution>
>                                 <id>test-stop-system-service</id>
>                                 <goals>
>                                     <goal>exec</goal>
>                                 </goals>
>                                 <phase>post-integration-test</phase>
>                                 <configuration>
>                                     <workingDirectory>../system</workingDirectory>
>                                     <executable>mvn${mvn.extension}</executable>
>                                     <arguments>
>                                         <argument>liberty:stop-server</argument>
>                                     </arguments>
>                                 </configuration>
>                             </execution>
>                         </executions>
>                     </plugin>
>                 </plugins>
>             </build>
>         </profile>

add page-seo-title and page-seo-description
add role="no_copy" to these:
"Teardown" section title needs to start with an action verb, so "Tearing...".

Re question about having unix/windows blocks split up, or having them together with a comment to distinguish them. Confirmed with @proubatsis that these commands are copy-able and user needs to run them. For that, I'd prefer them being split up for ease of use.

@Kubik42 I fixed the inconsistency in the pom file, I also fixed all the issues you brought up. Can you verify that all of your concerns have been addressed please?

Here are my final comments:

Why use Istio?

Can you briefly state what each feature could be used for? For examples" Routing for routing between different versions of a service. Logging for tracing requests across systems. Etc."

Starting and preparing your cluster for Deployment

For "kubectl apply" I wanted you to say something like "Next, deploy Istio's resource to your cluster by running the kubectl apply command, which creates Kubernetes resource from a yaml file". Or something like that.

Deploying version 1 of the services to cluster

"The kubernetes.yaml configuration file contains configuration for various Kubernetes resources" -> add code formatting to kubernetes.yaml
add headers to the table
write something after kubectl get deployments. Like "Which produces a list of Deployments for your two microservices"

Configure request routing

"Apply the routing rules you created in the routing.yaml file by running the given command."

Can you also address the last three points in my previous comment?

@Kubik42 https://github.com/OpenLiberty/draft-guide-istio/commit/d965c00a1afcc0579e1188a764693568d05b06e7

Add seo title and description
Add role no copy to example responses
Rename 'Teardown' section
Add sample use cases for features other than routing
Improve kubectl apply description
Add header to table
Describe 'kubectl get deployments'
Add proper formatting for 'kubernetes.yaml'
Format 'routing.yaml' appropriately

OpenLiberty / guide-istio-intro

Peer review #1