-
Notifications
You must be signed in to change notification settings - Fork 1
NPA-6550: docs for proxygen setup #125
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?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,85 @@ | ||||||||||||||
| # Setting up Proxygen | ||||||||||||||
|
|
||||||||||||||
| ## Generating a key pair | ||||||||||||||
|
|
||||||||||||||
| You'll need to generate a public and private key pair before raising a service now request to create the proxygen API | ||||||||||||||
|
||||||||||||||
| You'll need to generate a public and private key pair before raising a service now request to create the proxygen API | |
| You'll need to generate a public and private key pair before raising a ServiceNow request to create the proxygen API. |
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.
Worth saying YYYY-mm-dd should be the expiry date, not today's date. This confused me, from what's written
Copilot
AI
Apr 23, 2026
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.
The fenced code block for the Slack quote is malformed (the opening fence includes text: "Follow..."). This will render incorrectly in Markdown. Put the opening on its own line (optionally with a language like text) and move the quote content onto the next line.
Copilot
AI
Apr 23, 2026
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.
Grammar: "walkthrough" is used as a verb here; consider "walks through" or "walk through". Also "move onto" should be "move on to". These small fixes make the instructions read more cleanly.
| [These docs](https://digital.nhs.uk/developer/guides-and-documentation/security-and-authorisation/application-restricted-restful-apis-signed-jwt-authentication#step-2-generate-a-key-pair) walkthrough how to generate the key pair. Once you've created your JWKS file called YOUR_KID.json you can move onto creating the proxygen deployment | |
| [These docs](https://digital.nhs.uk/developer/guides-and-documentation/security-and-authorisation/application-restricted-restful-apis-signed-jwt-authentication#step-2-generate-a-key-pair) walk through how to generate the key pair. Once you've created your JWKS file called YOUR_KID.json you can move on to creating the proxygen deployment |
Copilot
AI
Apr 23, 2026
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.
"setup" is used as a verb here; in prose it should typically be "set up" (verb) vs "setup" (noun/adjective). Aligning the phrasing helps readability.
| Once APIM have done this, they will provide you with a client ID in the service now request, e.g. im1-pfs-auth-client which you will need to setup machine access to the API. | |
| Once APIM have done this, they will provide you with a client ID in the service now request, e.g. im1-pfs-auth-client which you will need to set up machine access to the API. |
Copilot
AI
Apr 23, 2026
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 example uses a specific developer machine path (/Users/elliot/...) and a specific Python version. Thatβs likely to confuse others and will go stale. Prefer using $HOME/~ and/or a more general way to locate the user-base bin directory so the instructions work across machines.
| You're looking for one that looks like `/Users/elliot/Library/Python/3.13/bin/proxygen` | |
| Once you have this path, you can add it to your .zshrc with `export PATH="$PATH:/Users/elliot/Library/Python/3.13/bin/"` | |
| You're looking for one that looks like `~/Library/Python/.../bin/proxygen` | |
| Once you have this path, add the directory containing it to your `.zshrc`, for example with `export PATH="$PATH:$(dirname "$(find ~ -name "proxygen" 2>/dev/null | head -n 1)")"` |
Copilot
AI
Apr 23, 2026
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.
Same "set up" vs "setup" issue: "To fully setup machine access" reads as a verb phrase, so "set up" would be clearer.
| To fully setup machine access, you will need a credentials.yaml and settings.yaml file inside of ~/.proxygen | |
| To fully set up machine access, you will need a credentials.yaml and settings.yaml file inside of ~/.proxygen |
Copilot
AI
Apr 23, 2026
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.
The base_url placeholder includes spaces and a | inside braces ({ptl | prod}), which makes it easy to accidentally copy/paste an invalid URL into credentials.yaml. Consider providing two explicit example URLs (one for PTL and one for PROD), or use a placeholder format without spaces/symbols that would break the URL.
Copilot
AI
Apr 23, 2026
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.
Same issue as base_url: endpoint_url uses a {ptl | prod} placeholder that will produce an invalid URL if copied verbatim. Provide explicit PTL/PROD examples or a safer placeholder format.
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.
The document title is "Setting up Proxygen" but the file is named
Rotating_proxygen_keys.md. This mismatch makes the doc harder to discover and link correctly. Consider renaming the file (or adjusting the title) so the filename reflects the actual content.