-
Notifications
You must be signed in to change notification settings - Fork 13
Update Readme for HMAC #192
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
Update Readme for HMAC #192
Conversation
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.
Pull Request Overview
This PR updates the README and release notes to introduce HMAC signing support, rename the key parameter to auth_key, and update documentation examples accordingly.
- Rename client parameter
keytoauth_keyand add a newsign_secretoption for HMAC. - Update release notes with upgrade instructions and new feature descriptions.
- Refresh README examples to show environment-based secret retrieval and CLI usage.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| RELEASE_NOTES.md | Added upgrade note for auth_key rename and detailed the new sign_secret CLI/client flags. |
| README.md | Imported os, updated the Python example to load sign_secret from env, and extended CLI usage. |
Comments suppressed due to low confidence (3)
README.md:164
- The CLI flag
--keyremains even though the client constructor parameter was renamed toauth_key. Consider renaming this flag to--auth_key(or documenting that--keymaps toauth_key) for consistency.
--key=$(<api_key.txt)
README.md:164
- The example is missing a trailing backslash (
\) after this line, so the subsequent--sign_secretline won't be interpreted as part of the same command.
--key=$(<api_key.txt)
README.md:52
- The environment variable name
REPORTING_API_SIGN_SECRETdiffers from the shell variableSIGN_SECRETused in the CLI example; consider using the same name in both contexts or adding a note to clarify the mapping.
SIGN_SECRET= os.environ['REPORTING_API_SIGN_SECRET']
README.md
Outdated
| ```bash | ||
| reporting-cli \ | ||
| --url localhost:4711 \ | ||
| --key=$(<api_key.txt) |
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 has also changed
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.
Ah, right. I checked at the wrong place and there is actually a bug here I need to fix :)
README.md
Outdated
|
|
||
| # Change server address | ||
| SERVER_URL = "grpc://replace-this-with-your-server-url:port" | ||
| API_KEY = open('api_key.txt').read().strip() |
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 suggest we also get this from the env to make it consistent with the way we pass in the the secret.
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.
Gladly
1ca50fd to
72474fd
Compare
RELEASE_NOTES.md
Outdated
| * Add HMAC generation capabilities. | ||
| * The new CLI option "key" can be used to provide the server's key. | ||
| * The client itself now has a "key" argument in the constructor. | ||
| * The new CLI option "sign_secret" can be used to provide the server's key. |
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.
It's confusing to read that the key should go into "sign_secret" when there is also "auth_key" formerly key 😅
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.
okay, that one is definitely confusing and should be fixed in the changelog
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.
Fixed it to be clear that it's the server's secret (aka API secret)
Signed-off-by: Florian Wagner <[email protected]>
Signed-off-by: Florian Wagner <[email protected]>
72474fd to
7c758ac
Compare
No description provided.