Skip to content

Regenerate Track 2 KeyVault libraries with new separated 7.1 specs#7299

Merged
daviwil merged 7 commits intoAzure:masterfrom
daviwil:regen-keyvault-split
Mar 6, 2020
Merged

Regenerate Track 2 KeyVault libraries with new separated 7.1 specs#7299
daviwil merged 7 commits intoAzure:masterfrom
daviwil:regen-keyvault-split

Conversation

@daviwil
Copy link
Contributor

@daviwil daviwil commented Feb 8, 2020

This change regenerates the Track 2 KeyVault data plane libraries with the new 7.1 specs that have been separated so that individual services have their own Swagger files. I've also added AutoRest configuration files and npm scripts so that it's easy for anyone to regenerate these in the future.

It appears that there have been some slight changes to the keys.json spec that makes the generated code incompatible with the convenience layer:

src/cryptographyClient.ts:66:7 - error TS2322: Type 'import("/home/daviwil/tmp/keyvault/azure-sdk-for-js/sdk/keyvault/keyvault-keys/src/core/models/index").JsonWebKey' is not assignable to type 'import("/home/daviwil/tmp/keyvault/azure-sdk-for-js/sdk/keyvault/keyvault-keys/src/keysModels").JsonWebKey'.
  Types of property 'kty' are incompatible.
    Type '"EC" | "EC-HSM" | "RSA" | "RSA-HSM" | "oct" | "oct-HSM" | undefined' is not assignable to type '"EC" | "EC-HSM" | "RSA" | "RSA-HSM" | "oct" | undefined'.
      Type '"oct-HSM"' is not assignable to type '"EC" | "EC-HSM" | "RSA" | "RSA-HSM" | "oct" | undefined'.

66       return key.key!;
         ~~~~~~~~~~~~~~~~

src/index.ts:1135:7 - error TS2322: Type 'import("/home/daviwil/tmp/keyvault/azure-sdk-for-js/sdk/keyvault/keyvault-keys/src/core/models/index").JsonWebKey | undefined' is not assignable to type 'import("/home/daviwil/tmp/keyvault/azure-sdk-for-js/sdk/keyvault/keyvault-keys/src/keysModels").JsonWebKey | undefined'.
  Type 'import("/home/daviwil/tmp/keyvault/azure-sdk-for-js/sdk/keyvault/keyvault-keys/src/core/models/index").JsonWebKey' is not assignable to type 'import("/home/daviwil/tmp/keyvault/azure-sdk-for-js/sdk/keyvault/keyvault-keys/src/keysModels").JsonWebKey'.

1135       key: keyBundle.key,
           ~~~

  src/keysModels.ts:148:3
    148   key?: JsonWebKey;
          ~~~
    The expected type comes from property 'key' which is declared here on type 'KeyVaultKey & DeletedKey'

src/index.ts:1138:7 - error TS2322: Type 'string[] | undefined' is not assignable to type 'KeyOperation[] | undefined'.
  Type 'string[]' is not assignable to type 'KeyOperation[]'.
    Type 'string' is not assignable to type 'KeyOperation'.

1138       keyOperations: keyBundle.key ? keyBundle.key.keyOps : undefined,
           ~~~~~~~~~~~~~

  src/keysModels.ts:166:3
    166   keyOperations?: KeyOperation[];
          ~~~~~~~~~~~~~
    The expected type comes from property 'keyOperations' which is declared here on type 'KeyVaultKey & DeletedKey'

src/index.ts:1139:7 - error TS2322: Type '"EC" | "EC-HSM" | "RSA" | "RSA-HSM" | "oct" | "oct-HSM" | undefined' is not assignable to type '"EC" | "EC-HSM" | "RSA" | "RSA-HSM" | "oct" | undefined'.
  Type '"oct-HSM"' is not assignable to type '"EC" | "EC-HSM" | "RSA" | "RSA-HSM" | "oct" | undefined'.

1139       keyType: keyBundle.key ? keyBundle.key.kty : undefined,
           ~~~~~~~

  src/keysModels.ts:162:3
    162   keyType?: KeyType;
          ~~~~~~~
    The expected type comes from property 'keyType' which is declared here on type 'KeyVaultKey & DeletedKey'

Marking this as DRAFT until the requisite spec files have been merged into the azure-rest-api-specs repo (PR Azure/azure-rest-api-specs#8301).

@daviwil daviwil requested review from heaths and sadasant February 8, 2020 00:02
@heaths
Copy link
Member

heaths commented Feb 8, 2020

If I'm parsing the errors right in your comment, these are expected until the convenience layer is updated, since "oct-hsm" and "import" enum values were added. Was that all?

Copy link
Member

@heaths heaths left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reviewed the smaller diffs in full, and sampled the larger commits - all deletions as expected. Looks good.

@daviwil daviwil requested a review from sophiajt February 10, 2020 17:56
@daviwil
Copy link
Contributor Author

daviwil commented Feb 10, 2020

Adding @jonathandturner to spot-check the type coercions I added in commit 7319251 😄

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

woah!! Is this all that is needed to regenerate this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In theory, yes! I would certainly appreciate if you could clone the branch and give it a shot just to make sure.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm trying it now!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It says No input files provided. Am I missing something?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Forgot to commit the swagger/README.md files, fixed now :/

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thank you! It works well!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The diff of this file is messy. I'm guessing we're now sorting the methods differently? I want to understand. Is this expected?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's expected because we're removing a large number of functions from this file which are no longer part of the spec used for @azure/keyvault-certificates The order of functions is the same, I think Git just can't figure out the proper way to diff them since the overall file structure has a lot of overlap.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it! Thank you! This PR works well!

Copy link
Contributor

@sadasant sadasant left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! And runs good as well, locally.

@daviwil daviwil force-pushed the regen-keyvault-split branch from 5c70302 to 88ec1a1 Compare March 6, 2020 19:25
@daviwil daviwil marked this pull request as ready for review March 6, 2020 20:03
@daviwil daviwil requested a review from bterlson as a code owner March 6, 2020 20:03
@daviwil
Copy link
Contributor Author

daviwil commented Mar 6, 2020

I've regenerated everything here in response to the final spec changes in #7299. If tests go green after the latest CI run, I'll merge it.

Thanks all!

@daviwil daviwil merged commit 10486cf into Azure:master Mar 6, 2020
@daviwil daviwil deleted the regen-keyvault-split branch March 6, 2020 20:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants