[keyvault] samples updates for doc publishing by willmtemple · Pull Request #6626 · Azure/azure-sdk-for-js

willmtemple · 2019-12-18T18:32:00Z

Similar to #6496 but for keyvault. Addresses #5679 CC @sadasant

Fixes #6625

Changes:

Made keyvault samples compatible with run-samples (note they don't run yet in series so they are disabled see: [keyvault] Samples cause conflicts preventing them from running one after another #6625 )
Removed build:samples from the full build (prep-samples is a destructive script, so this is disabled for now)
added all the metadata for readmes, package.json files, etc. for samples

NOTES by @sadasant:

If you want to merge this, move it back to where Will left it (here: fbb3bf1) (note from Will: did this) by undoing my commits and merge it. Though, this PR won't run anything on CI until we address the issues I'm trying to fix with my commits.
This currently won't work on CI (see this link). Just like we have environments with TestType node and browser on the CI resources (tests.yml et al), we need to add at least one environment with TestType sample as part of this PR, then re-run the pipelines.

…fault. - Sample build is potentially destructive, so disabling sample build until it can be made non-destructive.

… making destructive changes

sophiajt · 2019-12-18T21:28:04Z

sdk/keyvault/keyvault-certificates/samples/javascript/mergeCertificate.js

  await client.mergeCertificate("MyCertificate", [Buffer.from(base64Crt)]);
 }

+module.exports = { main };


This was mentioned in the TypeScript meeting. Just wanted to throw my vote in for autogenerating this so it's easier for people to take examples straight from the repo.

Yes, I'd like to get rid of this and modify the prep-samples script to insert it, but I will have to do it for every sample at the same time in another PR. I could make changes to the storage ones in this PR, but I have another PR for some fixes to storage samples open right now so I think it would be best to do them all mechanically in a third PR.

Let's use a separate PR to make that change in both keyvaults and storage at the same time and use this PR to get keyvault samples to be in a runnable state by scripts

sophiajt · 2019-12-18T21:28:36Z

sdk/keyvault/keyvault-certificates/samples/javascript/mergeCertificate.js

 const { DefaultAzureCredential } = require("@azure/identity");

+// Load the .env file if it exists
+require("dotenv").config();


Would it be possible to auto-generate this?

The dotenv bits are for end-users of the samples. CI won't need them. The sample readme presents use of dotenv as an option, so this part has to be checked in for that to work when a customer downloads the sample zip file.

Thinking through the concepts we're asking the user to understand to use the samples, I wonder if we should just mention this in the docs but not in the samples to keep the samples "as simple as possible".

It's nice to have the option to use .env if that's what the user wants to do, but if they'd rather just update the environment where their app lives, they don't actually need the .env part for the sample to work.

ramya-rao-a · 2019-12-20T17:45:09Z

/azp run js - keyvault - ci

azure-pipelines · 2019-12-20T17:45:22Z

Azure Pipelines successfully started running 1 pipeline(s).

sadasant · 2019-12-20T22:33:25Z

/azp run js - keyvault-keys - tests

azure-pipelines · 2019-12-20T22:33:36Z

Azure Pipelines successfully started running 1 pipeline(s).

sadasant · 2019-12-20T22:44:59Z

I added [DO NOT MERGE] on the title since to make this work I had to delete some .js samples (not their .ts counterparts). This needs to be investigated.

willmtemple · 2019-12-20T23:06:49Z

Storage packages have an environment for executing the samples in Linux that's configured as part of the tests pipeline. As far as I know, they're the only ones. However, in storage, those pipelines don't have the environment variables for AAD rbac, so the AAD tests are skipped there. Here in keyvault, we'll need to make sure that they are available since all the samples use them.

…update

willmtemple · 2020-01-06T18:21:12Z

I'm going to rewrite history here so that we can merge the samples and get them live, then we can worry about the CI part of it later once that infrastructure is in place.

CC @sadasant

sadasant · 2020-01-06T19:37:50Z

/azp run js - keyvault-keys - tests

azure-pipelines · 2020-01-06T19:38:03Z

Azure Pipelines successfully started running 1 pipeline(s).

…update

sophiajt · 2020-01-06T22:52:48Z

sdk/keyvault/keyvault-certificates/package.json

    "build:nodebrowser": "rollup -c 2>&1",
    "build:test": "npm run build:es6 && rollup -c rollup.test.config.js 2>&1",
-    "build": "npm run extract-api && npm run build:samples && npm run build:es6 && npm run build:nodebrowser",
+    "build": "npm run extract-api && npm run build:es6 && npm run build:nodebrowser",


I'd like to keep building the samples in here in some capacity so that if we break the samples the CI will warn us.

sophiajt · 2020-01-06T22:54:19Z

sdk/keyvault/keyvault-certificates/samples/javascript/backupAndRestore.js

 const { DefaultAzureCredential } = require("@azure/identity");

+// Load the .env file if it exists
+require("dotenv").config();


Going forward, are we recommending users put this in their projects?

sophiajt · 2020-01-06T22:58:01Z

sdk/keyvault/keyvault-certificates/samples/javascript/mergeCertificate.js

 const { DefaultAzureCredential } = require("@azure/identity");

+// Load the .env file if it exists
+require("dotenv").config();


Thinking through the concepts we're asking the user to understand to use the samples, I wonder if we should just mention this in the docs but not in the samples to keep the samples "as simple as possible".

It's nice to have the option to use .env if that's what the user wants to do, but if they'd rather just update the environment where their app lives, they don't actually need the .env part for the sample to work.

witemple-msft added 9 commits December 16, 2019 15:52

[keyvault] Modify samples to work with prep-samples

8065b4d

[keyvault-certificates] Samples updated to new runner

25a97dd

[keyvault] Build scripts for samples updated. Disable sample build de…

0d9d147

…fault. - Sample build is potentially destructive, so disabling sample build until it can be made non-destructive.

[prep-samples] Use git status to check if sample tree is dirty before…

0efbd33

… making destructive changes

[keyvault-keys] Added sample package data.

63aaa0f

[keyvault-certificates] Updated reamde with keyvault provisioning link

11b4889

[keyvault-keys] Added dotenv configuration to samples

01e4c19

[keyvault-secrets] Add samples package metadata and update samples

70b32e2

[keyvault] Fixed a link in keys/certificates sample readmes

fbb3bf1

willmtemple requested review from KarishmaGhiya, bterlson, mikeharder, sadasant and sophiajt as code owners December 18, 2019 18:32

sophiajt reviewed Dec 18, 2019

View reviewed changes

willmtemple self-assigned this Dec 19, 2019

willmtemple added the KeyVault label Dec 19, 2019

willmtemple requested review from danieljurek and richardpark-msft as code owners December 20, 2019 22:32

sadasant changed the title ~~[keyvault] samples updates for doc publishing~~ [DO NOT MERGE] [keyvault] samples updates for doc publishing Dec 20, 2019

Merge remote-tracking branch 'upstream/master' into keyvault-samples-…

21baee6

…update

willmtemple force-pushed the keyvault-samples-update branch from 97b9031 to 21baee6 Compare January 6, 2020 18:21

willmtemple changed the title ~~[DO NOT MERGE] [keyvault] samples updates for doc publishing~~ [keyvault] samples updates for doc publishing Jan 6, 2020

witemple-msft and others added 5 commits January 6, 2020 12:13

Merge remote-tracking branch 'upstream/master' into keyvault-samples-…

46a5549

…update

small listOperations fix

1148251

unique names in secret samples

bbcdc64

more unique names

8e095a1

certificate sample names and some package.json changes

aea49f8

sophiajt reviewed Jan 6, 2020

View reviewed changes

sadasant approved these changes Jan 7, 2020

View reviewed changes

willmtemple merged commit 6dc4c13 into Azure:master Jan 7, 2020

sadasant deleted the keyvault-samples-update branch January 8, 2020 15:21

Conversation

willmtemple commented Dec 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ramya-rao-a commented Dec 20, 2019

Uh oh!

azure-pipelines bot commented Dec 20, 2019

Uh oh!

sadasant commented Dec 20, 2019

Uh oh!

azure-pipelines bot commented Dec 20, 2019

Uh oh!

sadasant commented Dec 20, 2019

Uh oh!

willmtemple commented Dec 20, 2019

Uh oh!

willmtemple commented Jan 6, 2020

Uh oh!

sadasant commented Jan 6, 2020

Uh oh!

azure-pipelines bot commented Jan 6, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

willmtemple commented Dec 18, 2019 •

edited

Loading