-
Notifications
You must be signed in to change notification settings - Fork 102
NGF: Update user architecture guide #953
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
base: main
Are you sure you want to change the base?
Conversation
424e4c5
to
7371e09
Compare
61807f5
to
6d7564c
Compare
Does this have anything specific to our 2.1 release? If not, it can target main. |
don't we want these changes in release 2.1 ? Does not have anything specific to release 2.1. |
If we publish to main, then it becomes public immediately. It's not inherently tied to the release, so we could publish it as soon as it's done. And if this doesn't get merged before we release, then the 2.1 branch will be deleted, so we would merge to main anyway. |
Thanks for clarifying, I thought the release branch is our latest branch on changes that get reflected. |
Nope, the main branch is what gets published. The release branch is for changes upcoming in our release, that will be merged into main when we release and those features become available. |
6d7564c
to
f7c965a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally LGTM, added edit suggestions for sentence case headings.
There's also an ambiguous sentence that should be reworked.
--- | ||
|
||
## High-Level Example of NGINX Gateway Fabric in Action | ||
## High-Level Overview of NGINX Gateway Fabric in Action | ||
|
||
This figure depicts an example of NGINX Gateway Fabric exposing three web applications within a Kubernetes cluster to clients on the internet: | ||
|
||
```mermaid | ||
graph LR |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
couple notes on this diagram:
Let's change the NGF Pod name to NGF Control Plane Pod, and NGINX Pod to NGINX Data Plane Pod, to reinforce the concepts that we already introduced in this doc.
Also, the NGF Pod is outside the Kubernetes cluster. It should be inside it, in the nginx-gateway namespace.
(same goes for the table that follows the diagram)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ADubhlaoich wanted [overview](https://github.com/nginx/documentation/pull/953#discussion_r2275964667)
so I'll keep it that way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I"m referring to the diagram, not the overview change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh my bad, i confused it with the diff
class DataplaneComponentsAB dashedSubgraph; | ||
class DataplaneComponentsC dashedSubgraph; | ||
``` | ||
| **Category** | **Description** | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We technically should not be using the NGF
acronym in our docs, so let's replace that anywhere with NGINX Gateway Fabric.
@@ -260,125 +181,95 @@ For example, the Cluster Operator is denoted by the color green, indicating they | |||
graph LR |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar comments for this diagram and table
Does the rendered version with tables look okay? Not seeing the automatic preview in the PR... |
Co-authored-by: Alan Dooley <github@adubhlaoi.ch> Co-authored-by: Saylor Berman <s.berman@f5.com>
let me add some screenshots for visibility |
Screenshots to view rendered diagrams and tables @sjberman ![]() ![]() ![]() |
As a point of record, there's no automatic preview because the branch for the PR is from a fork. F5 employees can just branch directly from the repo without forking, which will generate a preview. |
Proposed changes
Updates user architecture guide to be less verbose and more readable with high level overview
Checklist
Before sharing this pull request, I completed the following checklist:
Footnotes
Potentially sensitive information includes personally identify information (PII), authentication credentials, and live URLs. Refer to the style guide for guidance about placeholder content. ↩