Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[core] core-lro depends on core-http #15880

Closed
maorleger opened this issue Jun 21, 2021 · 1 comment · Fixed by #15884
Closed

[core] core-lro depends on core-http #15880

maorleger opened this issue Jun 21, 2021 · 1 comment · Fixed by #15884
Assignees
@xirzec
Labels
Azure.Core Client This issue points to a problem in the data-plane of the library.
Milestone
[2021] July

Comments

@maorleger
Copy link
Member

maorleger commented Jun 21, 2021 •
edited
Loading

Discovered when moving Azure Key Vault to core V2. Looks like core-lro still takes a dependency on core-http

Error when building after removing core-http:

TypeError: Cannot read property 'prototype' of undefined
    at extend (/home/mleger/workspace/worktrees/keyvault-admin-core-v2/common/temp/node_modules/.pnpm/xmlbuilder@11.0.1/node_modules/xmlbuilder/lib/XMLElement.js:4:195)
    at /home/mleger/workspace/worktrees/keyvault-admin-core-v2/common/temp/node_modules/.pnpm/xmlbuilder@11.0.1/node_modules/xmlbuilder/lib/XMLElement.js:18:5
    at /home/mleger/workspace/worktrees/keyvault-admin-core-v2/common/temp/node_modules/.pnpm/xmlbuilder@11.0.1/node_modules/xmlbuilder/lib/XMLElement.js:296:4
    at /home/mleger/workspace/worktrees/keyvault-admin-core-v2/common/temp/node_modules/.pnpm/xmlbuilder@11.0.1/node_modules/xmlbuilder/lib/XMLElement.js:298:4
    at createCommonjsModule (/home/mleger/workspace/worktrees/keyvault-admin-core-v2/sdk/keyvault/keyvault-admin/dist-test/index.node.js:68:35)
    at Object.<anonymous> (/home/mleger/workspace/worktrees/keyvault-admin-core-v2/sdk/keyvault/keyvault-admin/dist-test/index.node.js:29765:18)
    at Module._compile (internal/modules/cjs/loader.js:1063:30)

Looking up who is asking for xmlbuilder:

keyvault-admin keyvault-admin-core-v2 % npm ls xmlbuilder
@azure/keyvault-admin@4.1.0-beta.1 /home/mleger/workspace/worktrees/keyvault-admin-core-v2/sdk/keyvault/keyvault-admin
└─┬ @azure/core-lro@1.0.6 -> /home/mleger/workspace/worktrees/keyvault-admin-core-v2/sdk/core/core-lro
  └─┬ @azure/core-http@1.2.7
    └─┬ xml2js@0.4.23
      └── UNMET DEPENDENCY xmlbuilder@~11.0.0

Finally, Looking at core-lro it includes core-http in dependencies.

Looking at the codebase it looks like none of the product code actually needs core-http, but it is used in tests.

Anyway, I think we should remove the core-http dependency from core-lro or at least move it to devDependencies?
@ghost ghost added the needs-triage Workflow: This is a new issue that needs to be triaged to the appropriate team. label Jun 21, 2021
@maorleger maorleger mentioned this issue Jun 21, 2021
Merged
@xirzec
Copy link
Member

xirzec commented Jun 21, 2021

If core-lro doesn't have a real runtime dependency on core-http it absolutely should not depend on it

@xirzec xirzec self-assigned this Jun 21, 2021
xirzec added a commit to xirzec/azure-sdk-for-js that referenced this issue Jun 21, 2021
@xirzec
Remove core-lro runtime dependency on core-http
40dcfb9 
Fixes Azure#15880
@xirzec xirzec added Azure.Core Client This issue points to a problem in the data-plane of the library. labels Jun 21, 2021
@ghost ghost removed the needs-triage Workflow: This is a new issue that needs to be triaged to the appropriate team. label Jun 21, 2021
@xirzec xirzec mentioned this issue Jun 21, 2021
Merged
@ramya-rao-a ramya-rao-a added this to the [2021] July milestone Jun 21, 2021
xirzec added a commit that referenced this issue Jun 22, 2021
@xirzec
Remove core-lro runtime dependency on core-http (#15884)
b26a218 
Fixes #15880
maorleger added a commit that referenced this issue Jun 23, 2021
@maorleger
[KeyVault] - Migrate Key Vault Admin package to Core V2 (#15881)
b499bfb 
## What

- Migrate @azure/keyvault-admin to core V2
- Migrate `KeyVaultBackupClient` and `KeyVaultAccessControlClient` to core CAE
- Bump our minimum `@azure/core-lro` version to 1.0.6

## Why

This PR proves out two important things: it demonstrates that core continuous access evaluation works for both container 
registry (already done) and Key Vault (this PR). It also demonstrates the migration path for Core V2 for Key Vault.

The change to core-lro addresses an issue where core-lro was incorrectly depending on core-http (#15880)  That has been fixed on 1.0.6 
and allows package owners to migrate to core-rest-pipeline and remove core-http without seeing build breaks.


Resolves #15522 
Resolves #14306
@github-actions github-actions bot locked and limited conversation to collaborators Apr 11, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@xirzec xirzec

Labels
Azure.Core Client This issue points to a problem in the data-plane of the library.
Projects
None yet
Milestone
[2021] July
Development

Successfully merging a pull request may close this issue.

[core] Remove core-lro runtime dependency on core-http xirzec/azure-sdk-for-js
3 participants
@xirzec @maorleger @ramya-rao-a