-
Notifications
You must be signed in to change notification settings - Fork 125
Docs: add missing prerequisite for installation #1485
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
|
✅ All required contributors have signed the F5 CLA for this PR. Thank you! |
✅ Deploy Preview will be available once build job completes!
|
|
I have hereby read the F5 CLA and agree to its terms |
d366350 to
dc87dc4
Compare
c0adb94 to
d54baa0
Compare
d54baa0 to
9f5a581
Compare
JTorreG
left a comment
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.
There is a TODO label
dfc4c24 to
04698aa
Compare
ADubhlaoich
left a comment
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: approval pending feedback.
Please run the linting tools on this branch, which can be executed automatically with pre-commit.
ADubhlaoich
left a comment
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.
Some more minor changes for content flow and formatting.
5645ca7 to
23eddc1
Compare
Co-authored-by: yar <y82@users.noreply.github.com>
Co-authored-by: yar <y82@users.noreply.github.com>
Co-authored-by: yar <y82@users.noreply.github.com>
Co-authored-by: yar <y82@users.noreply.github.com>
Co-authored-by: yar <y82@users.noreply.github.com>
Co-authored-by: yar <y82@users.noreply.github.com>
Co-authored-by: yar <y82@users.noreply.github.com>
Co-authored-by: yar <y82@users.noreply.github.com>
Co-authored-by: yar <y82@users.noreply.github.com>
ea72132 to
cc0a686
Compare
ADubhlaoich
left a comment
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 have already given this feedback and it has not been addressed: there has been unnecessary detail added to every single set of pre-requisites.
- Download the SSL certificate and private key associated with your F5 NGINX App Protect WAF subscription from the MyF5 Customer Portal.
Docker registry credentials are needed to access private-registry.nginx.com
These are not pre-requisites: they are instructions, and they are redundant instructions.
Immediately after the user finishes reading the "Before you begin" section, they are explicitly guided through the steps involved in downloading and configuring the credentials needed to follow the rest of the document.
content/waf/install/docker.md
Outdated
| {{< call-out "note" >}} | ||
| If you are using NGINX Open Source for your Multi-container or Hybrid configuration, you do not need the JWT license file. | ||
| {{< /call-out >}} | ||
|
|
||
| {{< include "licensing-and-reporting/download-jwt-ssl-key-from-myf5.md" >}} | ||
|
|
||
| {{< call-out "important" >}} | ||
| The provided Dockerfile for NGINX Plus automatically handles placing the JWT license file in `/etc/nginx/` during image build. If you use a custom Dockerfile, you must ensure the JWT license is copied to this location. | ||
| {{< /call-out >}} | ||
|
|
||
| {{< call-out "note" >}} Starting from [NGINX Plus Release 33]({{< ref "nginx/releases.md#r33" >}}), a JWT file is required for each NGINX Plus instance. For more information, see [About Subscription Licenses]({{< ref "/solutions/about-subscription-licenses.md">}}). {{< /call-out >}} |
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.
This is an excessive use of consecutive call-outs.
Call-outs are intended to draw the user's attention to something highly specific they need to take extra care of: if you over-use them, everything looks important, and nothing looks important as a result.
- The content of the first can be replaced with an anchor link to "skip" the unnecessary section for OSS users
- The second call-out is fine, and a good example of how call-outs should primarily be used
- The third call-out is redundant, because the instructions involving NGINX Plus guide the user towards using a JWT file: there's no point telling someone they will need to do something if you're already expecting them to do it as part of the instructions
@ADubhlaoich Are you able to speak with Aviv Dahan about this. This is something product requested. |
Proposed changes
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. ↩