Rework todo-backend README due to Glow, and due to similar content fo…#1163
Rework todo-backend README due to Glow, and due to similar content fo…#1163emmartins wants to merge 1 commit into
Conversation
…r both wfly and eap
kstekovi
left a comment
kstekovi
left a comment
There was a problem hiding this comment.
Thanks for the cleanup! Reducing the ifdef/ifndef duplication and reflecting the Glow-based provisioning is a clear improvement. I have a couple of issues to flag before this is ready to merge.
| $ helm dependency update charts | ||
| ---- | ||
|
|
||
| = Environment variables for PostgreSQL |
There was a problem hiding this comment.
The section heading uses a level-0 title (= Environment variables for PostgreSQL), but the original in additional-readme-cloud.adoc used === (level 3 subsection). When this file is included inside the cloud deployment sections (with leveloffset applied by the shared docs), this will render at an incorrect level in the output. Please change it to ===.
| :helm-install-prerequisites-openshift: ../todo-backend/helm-install-prerequisites.adoc | ||
| include::../shared-doc/build-and-run-the-quickstart-with-openshift.adoc[leveloffset=+1] | ||
| include::../todo-backend/additional-readme-openshift.adoc[leveloffset=+1] | ||
| :!additional-readme-openshift: |
There was a problem hiding this comment.
:additional-readme-openshift: is correctly unset after the include, but helm-install-prerequisites-openshift (set on the line above) is never unset. This leaks the attribute into subsequent sections. Please add :!helm-install-prerequisites-openshift: here (and similarly :!helm-install-prerequisites-kubernetes: after the Kubernetes section) to mirror the handling of the other attributes.
| in the secret `postgres-configmap`. | ||
|
|
||
| ifdef::additional-readme-openshift[] | ||
| == Use the todobackend Web Frontend |
There was a problem hiding this comment.
This "Use the todobackend Web Frontend" section contained important practical guidance for OpenShift users — specifically how to retrieve the deployed route URL and construct the full path to use with the todobackend.com specs and client:
$ oc get route todo-backend -o jsonpath="{.spec.host}"The todobackend.com links have been moved to the local testing section, but this cloud-specific URL discovery step is now missing. Without it, OpenShift users won't know how to find the URL to point todobackend.com at their deployed app. Could you add this back somewhere in the OpenShift section?
| deploy: | ||
| env: | ||
| - name: POSTGRESQL_PASSWORD | ||
| - name: POSTGRESQL_DATABASE |
There was a problem hiding this comment.
Minor/pre-existing: - name: POSTGRESQL_DATABASE is indented as a sub-item of - name: POSTGRESQL_PASSWORD rather than at the same level. Since this block is being moved and touched anyway, it would be a good opportunity to fix the indentation.
2 participants
…r both wfly and eap