Peer review - Windows

[jj-zhang]

Peer Review: review to be done by a peer member.

• Check that the guide has working instructions and sample code
• Check the quality of code according to the best coding practices
• Check the quality and presentation of guide according to Structure and Style Guideline
• Check the consistency of guide with template and other guides
• Examples with the right outcomes are provided
• Example with the wrong outcomes, if any, are provided
• Check that all licensing statements are properly stated in all files
• Check that the pom.xml, server.xml, etc files are clean
• Check that the directories are properly structured
• Test the guide on 3 platforms: Mac, Win, Linux
• If any URL visits, try them on different browsers: Firefox, Chrome, Safari
• If any URL visits, try curl command where applicable
• Ensure that the guide is using the latest version of the liberty-maven-plugin or liberty-gradle-plugin where applicable
• Check that some of these page-tags are used in a guide: MicroProfile, Maven, Docker, Kubernetes, Gradle, Java EE, Security, Cloud. Only these tags are visible on the website. Latest list here.
• Check the attribution statement is accurate for the guide
• Verify the travisTest.sh script, if any, is accurate and consistent with other guides

Peer Testing: tests to be done by a peer member.

Guide’s contributor’s (if available, otherwise peer tester’s) responsibility:

• Additional tests where applicable:
  • Define test coverage and review with team (including guide contributor, if available)
  • Define detail test cases
  • Consider corner cases targeting the specific guide
  • Consider different platform tests, ie, Mac, Windows, Linux
  • Consider corner cases UI tests
  • Consider testing URL on all browsers, ie, FF, Chrome, Safari
  • Consider testing the curl command for URL visits
• Consider building with both Maven and Gradle build tools
• Testing with different IDEs, ie, Atom, Eclipse (Optional: VS.code, IntelliJ, Microclimate)
• Run Acrolinx Checker on draft (above 70 score approximately)
• Consider SEO title and description for the guides
• Ensure automated test is enable, set up with Travis CI, and able to schedule tests
• Run diff -r start/ finish/ and there's no differences
• Ensure that the automation tests are able to run when PR is created

Peer Tester’s responsibility:

• Check the appearance of the guide on test site for the following items:
  • Table of contents
  • Headings
  • Paragraphs
  • code snippets
  • outputs
  • links
  • hotspots
• Test the guide end-to-end with working instruction and sample code
• Perform all the defined test cases

[TrevCraw]

Running into an issue trying to get the mongo Docker container running. Could be a Windows related issue. @jimmy1wu and I are investigating.

[TrevCraw]

One of the issues was the command to create the container, docker run --name mongo-sample -v $(pwd)/assets:/etc/mongodb -p 127.0.0.1:27017:27017 -d mongo --config /etc/mongodb/mongodb.conf, did not work with Git Bash on Windows. (Info on issue here: https://stackoverflow.com/questions/50608301/docker-mounted-volume-adds-c-to-end-of-windows-path-when-translating-from-linux)

@jimmy1wu is going to add a fix for this, which is to add a / before $(pwd)

[TrevCraw]

The second issue with Docker is it cannot mount the assets folder due to permission issues. I am not able to share the C drive on my machine due to this issue with Docker: docker/for-win#616

The Docker team says there is a version that resolves the issue. I can try to give that a shot, but either way, this could be a pain for Windows developers.

[TrevCraw]

For the section Setting up the MongoDB configuration, I got confused.
First, I think we should state that the "mongodb.conf" file already exists in the "assets" folder.
Second, we should make it clear that an update to the file is only needed if you are going to use MongoDB on Docker. Based on the current setup, I took the instructions to mean "(Everyone) Update the mongodb.conf file AND if you are using Docker, change the provided path. Otherwise, no change is needed to the code provided on this page".

[TrevCraw]

Found a small issue with the "copy code block" option in the Generating a self-signed certificate section. The openssl command does not allow for the continuation of a successive command in the terminal, so the cat command is not run automatically. This differs from other guides where, when commands were grouped together in one code block, they all ran one after the other. Maybe these two commands should be split up into different code blocks?

It also might be good to mention that openssl will prompt the user for lots of information, including the Common Name (CN), which is important to set as localhost. That way the user knows to be on the lookout for it while openssl is repetitively asking them to fill in info (I totally glossed over the CN when openssl kept asking me for stuff).

[TrevCraw]

What you’ll learn section:

This microservice connects to MongoDB using TLS by using a CDI producer to inject a MongoDatabase and MicroProfile Config to make configuring MongoDB driver simple."

• Should say "...to make configuring the MongoDB driver simple." OR "...MicroProfile Config, which makes it simple to configure the MongoDB driver."

Not sure if this needs to be addressed or not, but if someone does not know what CDI or MicroProfile are, should we have links to other guides for them?

[TrevCraw]

MongoDB installation and setup section:

These should all be suggestions for Windows users doing local Mongo install for the purposes of this guide:

1. Do NOT install MongoDB as a service
2. No need to install MongoDB Compass Community
3. Need to add path to mongoDB bin to system's PATH

Database setup section:

If still planning to suggest using Docker for Windows, then this command docker exec -it mongo-sample bash needs to have winpty at the beginning.

[TrevCraw]

Try what you’ll build section:

...in the console, then it is ready for you check out the microservice...
should say "...ready for you to check out..."

Using TLS section:

Since you started your MongoDB server using the TLS...
should say "...using TLS..." OR "...using the TLS something (ie. configuration, settings, etc)..."

Basic CRUD operations section:

...will be used to return MongoCollection<TDocument>, a generic with a TDocument type parameter.
a generic what? Collection? Object?

Update section:

The MongoCollection.replaceOne() method is used update a specified document.
should say "...is used to update..."

Testing the service section:

In this case, the method creates a JAX-RS client, which makes HTTP requests to the crew service, it also retrieves the port number and microservice context for the Open Liberty server, creates testing crew member data and builds a base URL string that is used throughout the tests.
This is a bit of a run on sentence. It should probably be split into at least two with some more commas.

[TrevCraw]

Basic CRUD operations section:

The MongoDatabase.getCollection() method will be used...
The MongoDatabase.getCollection() link does not go to the method, but just to the Class it belongs to. The rest of the links in the guide go directly to the methods.

For this guide, you will be using Document as the type.
The Document link goes to a BsonDocument class page, which appears to be a different class than the Document class.

[TrevCraw]

No need for a MongoDB feature section:

The MongoDB driver should be bundled in your microservice. To do this with Maven you can use a dependency:
Should the guide use a hotspot here to point to the example in the pom.xml?

Running the tests section:

To see whether the tests detect a failure, add an assertion that you know fails, or change the existing assertion to an unexpected constant value.
Maybe suggest an assertion to change and what to change it to? And then hotspot it here?
An easy one to change is assertEquals(_TestIDs.size(), testMemberCount, to assertEquals(0, testMemberCount,

[TrevCraw]

Update section:

The fields in the request body (Name, Rank, and CrewID) should all start with lowercase letters. Otherwise users will hit a 400 Bad Request error.

[TrevCraw]

openliberty is used as a password in two different contexts. One as the keystore password for TLS and then again as the password for the sampleUser in Mongo. It may be best to use two different passwords to not confuse the user about the purpose of the passwords.

[TrevCraw]

One thing I found a little jarring was the terms used for CRUD (other than Delete) in the guide are not found on the OpenAPI UI (it uses POST, PUT, GET). Maybe the guide should say something like:

Test the CREATE endpoint through the OpenAPI UI using the POST action with the following as the request body:

Test the READ endpoint using the GET action in the OpenAPI UI

Test the UPDATE endpoint with the PUT action from the OpenAPI UI by...

[jimmy1wu]

@TrevCraw Please review again.

Some comments:

• local install of mongo has been removed as an option for the guide
• a generic what? Collection? Object? -> a generic is a type of java class
• gilbert said the guide should be ran from command prompt, instead of gitbash

[TrevCraw]

@jimmy1wu As I mentioned in Slack, I am not sure command prompt is a good fit for the guide given the commands provided.

[jimmy1wu]

@TrevCraw after talking to gilbert, i am going to see if there is an easy solution for this. otherwise i will have to add a message saying that users should use an alternative that comes with it like git bash or maybe powershell

[jimmy1wu]

feedback addressed, closing