[KeyVault Keys] Fixed the rollup warnings #11549

sadasant · 2020-09-29T12:15:13Z

Context: I did most of this while writing instructions for others. It made sense to me to make the pull request instead of putting more effort in the instructions.

Fixes #9867

This PR:

Copies the files using Node-specific features into files with the same name but ending in .browser.
Cleans up any Node-specific references from the new .browser files.
Maps these files in the package.json
Removes the unnecessary content from the rollup.base.config.js
Ensures things build correctly. No rollup warnings, no lint issues, etc.
Ensures tests pass.

It's my first time doing the .browser stuff, so
Feedback appreciated ✨

sdk/keyvault/keyvault-keys/src/localCryptography/algorithms.browser.ts

sadasant · 2020-10-02T21:02:19Z

@ramya-rao-a After having the time to read this again, I realized I did a very poor job of moving the algorithms.ts file! These new changes I'm working on will make a lot more sense! Thank you for your patience.

sadasant · 2020-10-02T22:46:24Z

sdk/keyvault/keyvault-keys/src/localCryptography/algorithms.browser.ts

+/**
+ * A plain object containing all of the locally supported algorithms.
+ */
+export const localSupportedAlgorithms: LocalSupportedAlgorithmsRecord = {


If we keep the current types as is, this contraption is necessary. As soon as we get into browser support, this will expand and be coherent.

sadasant · 2020-10-02T22:46:44Z

sdk/keyvault/keyvault-keys/src/localCryptography/algorithms.ts

@@ -80,77 +74,6 @@ const pipeAssertions = (...assertions: LocalAssertion[]): LocalAssertion => (...
  }
 };

-/**


These types were moved to localCryptography/models.ts.

sadasant · 2020-10-02T22:48:25Z

sdk/keyvault/keyvault-keys/src/localCryptography/models.ts

@@ -1,4 +1,98 @@
 // Copyright (c) Microsoft Corporation.
 // Licensed under the MIT license.

+import { JsonWebKey } from "../keysModels";


These types were just moved out of the algorithms.ts file.

…eded on algorithms.browser.ts

ramya-rao-a · 2020-10-04T02:12:29Z

sdk/keyvault/keyvault-keys/src/localCryptography/algorithms.browser.ts

+ * The list of known assertions so far.
+ * Assertions verify that the requirements to execute a local cryptography operation are met.
+ */
+export const assertions: Record<string, LocalAssertion> = {};


Please note, the below feedback is not a blocker for this PR, but we should look into this nevertheless and do any follow ups in a separate PR.

I was trying to figure out whey we need to export these assertions. We use these assertions only in the validate method in each algorithm. Since we dont have any algorithm in browser, this got me thinking as to why we even need it which then lead me to the tests in the file localCryptography.spect.ts file. So, a few questions:

When we run the tests in the file localCryptography.spect.ts in CI in the browser, what is assertions pointing to? If it is the assertions in the algorithms.ts file, then we are essentially not getting the coverage for "browser code". If it is the assertions in the algorithms.browser.ts file, then tests would fail because assertions is an empty object as seen above. This delves into a generic question around how do we expect our tests to work when the code is split between node and browser like how are are doing in this PR and how we have done in many other places

Looking at the tests themselves, it felt a little odd to me that we were testing assertions. Shouldnt we be testing that the right assertions are added to the alogrithms i.e. Wouldnt we get a more appropriate coverage by calling validate() on the different algorithms and checking if the expected error was thrown?

I like those ideas! After this release, I'll go back to this PR and make issues based on your feedback. Thank you 🌞

We don't currently run cryptography tests on the browser. I can take your validate feedback and work with it on a separate issue! Here's the issue: #11779

ramya-rao-a

Looks good. Please look at #11549 (comment)

ramya-rao-a · 2020-10-04T02:15:36Z

/azp run js - keyvault-keys - tests

azure-pipelines · 2020-10-04T02:15:47Z

Azure Pipelines successfully started running 1 pipeline(s).

sadasant · 2020-10-12T17:31:47Z

Merged since it was approved, we just released and to avoid this PR from getting stale.

Api Management - make /tenant endpoints ARM compliant in 2020-06-01-preview version (Azure#11549) * Adds base for updating Microsoft.ApiManagement from version stable/2019-12-01 to version 2020-06-01-preview * Updates readme * Updates API version in new specs and examples * Add support in API Management for Availability Zones (Azure#10284) * apim in azs * fix prettier check * PATCH should return 200 OK (Azure#10328) * add support for PATCH returning 200 OK * CI fixes prettier fix CI fixes part 2 * Password no longer a mandatory property when uploading Certificates * add missing x-ms-odata extension for filter support * +gatewayhostnameconfiguration protocol changes (Azure#10292) * [2020-06-01-preview] Update Oauth Server secrets Contract (Azure#10602) * Oauth server secrets contract * fix azureMonitor enum * API Management Service Deleted Services Resource (Azure#10607) * API Management Service Deleted Services Resource * Path fix * Lint + custom-words fixes * Location URI parameter for deletedservices Resource * GET for deletedservices by service name * Remove resourceGroupName from resource path * fixes * schema for purge operation * perttier applied * 204 response code added Co-authored-by: REDMOND\glfeokti <glfeokti@microsoft.com> * OperationNameFormat property added to Diagnostic contract (Azure#10641) * OperationNameFormat property added to Diagnostic contract * add azuremonitor to update contract Co-authored-by: REDMOND\glfeokti <glfeokti@microsoft.com> * [Microsoft.ApiManagement][2020-06-01-preview] Change Network Status response contract (Azure#10331) * Change Network Status response contract * Update examples for network status contract * ApiManagement - tenant/settings endpoints * ApiManagement - tenant/settings endpoints fix * ApiManagement - tenant/settings endpoints fix prettier * ApiManagement - tenant/settings endpoints fix 3 * ApiManagement - tenant/settings endpoints fix 4 * ApiManagement - tenant/settings endpoints fix 5 Co-authored-by: Samir Solanki <samirsolanki@outlook.com> Co-authored-by: maksimkim <maksim.kim@gmail.com> Co-authored-by: promoisha <feoktistovgg@gmail.com> Co-authored-by: REDMOND\glfeokti <glfeokti@microsoft.com> Co-authored-by: RupengLiu <rliu1211@terpmail.umd.edu> Co-authored-by: vfedonkin <vifedo@microsoft.com>

sadasant added 2 commits September 28, 2020 20:31

wip

01f179e

small lint fix

d38c845

sadasant requested review from ramya-rao-a and richardpark-msft September 29, 2020 12:15

sadasant requested a review from sophiajt as a code owner September 29, 2020 12:15

sadasant self-assigned this Sep 29, 2020

ghost added the KeyVault label Sep 29, 2020

sadasant mentioned this pull request Sep 29, 2020

[KeyVault Keys] Roll up warnings for browser #9867

Closed

ramya-rao-a reviewed Sep 30, 2020

View reviewed changes

sdk/keyvault/keyvault-keys/src/localCryptography/algorithms.browser.ts Outdated Show resolved Hide resolved

sdk/keyvault/keyvault-keys/src/localCryptography/algorithms.browser.ts Outdated Show resolved Hide resolved

sadasant added 2 commits October 2, 2020 18:10

moved the models out of the algorithms files

b7789ff

cleanup after moving the models

3d67616

sadasant commented Oct 2, 2020

View reviewed changes

made an oversight locally. CI was there to help me. Assertions was ne…

5ab2536

…eded on algorithms.browser.ts

ramya-rao-a reviewed Oct 4, 2020

View reviewed changes

ramya-rao-a approved these changes Oct 4, 2020

View reviewed changes

sadasant merged commit 4cf3b9c into Azure:master Oct 12, 2020

sadasant deleted the keyvault-keys/9867 branch October 12, 2020 17:30

sadasant mentioned this pull request Oct 12, 2020

[Key Vault Keys] Local cryptography test improvements. #11779

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[KeyVault Keys] Fixed the rollup warnings #11549

[KeyVault Keys] Fixed the rollup warnings #11549

sadasant commented Sep 29, 2020 •

edited

sadasant commented Oct 2, 2020

sadasant Oct 2, 2020

sadasant Oct 2, 2020

sadasant Oct 2, 2020

ramya-rao-a Oct 4, 2020 •

edited

sadasant Oct 4, 2020

sadasant Oct 12, 2020

ramya-rao-a left a comment •

edited

ramya-rao-a commented Oct 4, 2020

azure-pipelines bot commented Oct 4, 2020

sadasant commented Oct 12, 2020

                 }
               };
-              /**

[KeyVault Keys] Fixed the rollup warnings #11549

[KeyVault Keys] Fixed the rollup warnings #11549

Conversation

sadasant commented Sep 29, 2020 • edited

sadasant commented Oct 2, 2020

sadasant Oct 2, 2020

Choose a reason for hiding this comment

sadasant Oct 2, 2020

Choose a reason for hiding this comment

sadasant Oct 2, 2020

Choose a reason for hiding this comment

ramya-rao-a Oct 4, 2020 • edited

Choose a reason for hiding this comment

sadasant Oct 4, 2020

Choose a reason for hiding this comment

sadasant Oct 12, 2020

Choose a reason for hiding this comment

ramya-rao-a left a comment • edited

Choose a reason for hiding this comment

ramya-rao-a commented Oct 4, 2020

azure-pipelines bot commented Oct 4, 2020

sadasant commented Oct 12, 2020

sadasant commented Sep 29, 2020 •

edited

ramya-rao-a Oct 4, 2020 •

edited

ramya-rao-a left a comment •

edited