-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
[KeyVault] README improvements #7664
Conversation
Here's an attempt to improve our READMEs.
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.
How are you ensuring that all these snippets compile and run/work? I recommend doing something like what .NET has. See eng/Update-Snippets.ps1 in the .NET repo for an example. It updates READMEs and other sample files with code from runnable tests (typically just live tests, but wouldn't have to be limited that way).
sdk/keyvault/README.md
Outdated
- [Key Vault Keys Examples](https://github.com/Azure/azure-sdk-for-js/tree/master/sdk/keyvault/keyvault-keys/samples) | ||
- [Key Vault Secrets Examples](https://github.com/Azure/azure-sdk-for-js/tree/master/sdk/keyvault/keyvault-secrets/samples) | ||
- [Key Vault Certificates Examples](https://github.com/Azure/azure-sdk-for-js/tree/master/sdk/keyvault/keyvault-certificates/samples) | ||
- [KeyVault Keys Samples (JavaScript)](https://github.com/Azure/azure-sdk-for-js/tree/master/sdk/keyvault/keyvault-keys/samples/javascript) |
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.
Why absolute URLs? If relative, you can browse the samples in Code or other editors. Besides, this as written will always point to latest which may change if you refactor the examples again. Relative paths will remain correlated.
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.
That makes sense! I'll change it.
I have to run these manually. I believe we're working on a fix, but that's not on my scope. I'll make sure to follow when the people in charge reach to a conclusion. In the mean time, I have to run these manually. |
- [KeyVault Secrets Test Cases](./keyvault-secrets/test/) | ||
- [KeyVault Certificates Samples (JavaScript)](./keyvault-certificates/samples/javascript) | ||
- [KeyVault Certificates Samples (TypeScript)](./keyvault-certificates/samples/typescript) | ||
- [KeyVault Certificates Test Cases](./keyvault-certificates/test/) |
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.
Do we consider these "samples" grade (meaning consumers can read them and get started)? Might be worth just removing it.
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.
Yep, most of our tests are thought as samples. In fact, our samples are almost copies of our tests.
@@ -301,7 +303,7 @@ async function main() { | |||
main(); | |||
``` | |||
|
|||
### Get a certificate | |||
### Getting a KeyVault certificate |
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.
Nit: the product is "Key Vault", not "KeyVault".
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.
Lots of instances all over docs that should 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.
Sounds good! I'll do these changes.
Using `openssl`, you can retrieve the public certificate in | ||
PEM format by using the following command: | ||
|
||
openssl pkcs12 -in myCertificate.p12 -out myCertificate.crt.pem -clcerts -nokeys |
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.
Should be a code block using triple backticks. Doesn't need a language i.e. can just be preformatted text.
Here's an attempt to improve our READMEs.
I'm taking feedback from the docs team and I'm also mentioning information relevant to #7647.