[KeyVault Certificates Hotfix 4.0.1] Updating core-http and opentelemetry #8872
Conversation
sadasant
requested review from
KarishmaGhiya,
bterlson,
daviwil,
mikeharder and
sophiajt
as code owners
| "@azure/identity": "^1.0.0", | ||
| "@azure/identity": "1.1.0-preview.3", | ||
| "@azure/keyvault-keys": "^4.0.0", | ||
| "@azure/keyvault-secrets": "^4.0.0", |
There was a problem hiding this comment.
certificates has dev dependencies on both keys & secrets, so how is it that only secrets is being updated in this PR?
There was a problem hiding this comment.
The keys dependency should be removed, it doesn't do anything.
There was a problem hiding this comment.
I've made a PR to remove this dependency from keyvault-certificates: #8874 It's a draft, but it's something inconsequential, so we can go back to that when we have time.
There was a problem hiding this comment.
Shouldnt the min version of the secrets dependency be updated to 4.0.3 now that the tests wont work with older versions of 4.0.0 - 4.0.2? And if we are making that change to secrets, we might as well remove keys from here
| "@azure/core-tracing": "1.0.0-preview.7", | ||
| "@azure/core-tracing": "1.0.0-preview.8", | ||
| "@azure/logger": "^1.0.0", | ||
| "@opentelemetry/types": "^0.2.0", |
There was a problem hiding this comment.
With the changes to secrets in this PR, we will now have 4.0.1 of certificates depending on a non released version of secrets which doesnt feel right. Can we not release secrets first, and then update the package.json file for certificates to depend on the released version of secrets?
There was a problem hiding this comment.
This secret dependency is just for tests, it's a devDependency. We can for sure do what you say, though it will make no difference for our users.
There was a problem hiding this comment.
It is less about the users and more about the precedence we are setting around maintaining our code.
In the master branch, we have packages depending on other packages which are not yet released as well. But, this is fine as each of them are on their way to be released in the next applicable release cycle. This is indicated by the version number in the package.json of these packages which is greater than the released version.
In this hotfix branch, version 4.0.3 of secrets is being updated in place to make the tests of certificates work. Anyone looking at the 4.0.1 tag for keyvault-certificates in the future can misunderstand that 4.0.3 of secrets has the latest tracing support, when in reality, it is version 4.0.4 of secrets that will have the latest tracing support.
There was a problem hiding this comment.
Since we are being forced to change the package version of core-tracing to preview.7 in 2804e58?file-filters%5B%5D=.json to make rush download the right version, I guess my concern on package version and the code inside the package not matching is a ship that has sailed. :(
There was a problem hiding this comment.
It's a very odd scenario. keyvault-certificates in this branch is using an unpublished keyvault-secrets 4.0.3 as you say, which as you point out doesn't align with the published 4.0.3 hotfix that will go based on the other hotfix branch.
To detect this though, users would need to clone the repository and go back to the hash of the hotfix release of keyvault-certificates, in which they will have a local copy of keyvault-secrets that would appear (Unreleased) in the changelog.
If they read certificates from their NPM bundle, they will only see the ^4.0.0 dependency.
I don't think there's any rush in releasing this hotfix before the keys and secrets hotfix, so we can pretty much wait and, as you suggested, release keys and secrets first, then update the certificate hotfix branches.
An easy way to do this update can be to force rush to download the 4.0.3 hotfix by pinning down keys and secrets like I'm doing with core-tracing: 2804e58 though this "pin down" approach mostly makes sense for core-tracing since we want to avoid having to update all of core-tracing dependencies by not pulling the actually published 1.0.0-preview.8.
In short, up to you! We can even delay this decision and focus on releasing the hotfix for keys and secrets.
There was a problem hiding this comment.
Keep in mind that I am aware that I'm mostly repeating what you're saying, but I want to make sure I communicate what I understand instead of assuming things.
|
I'm in communications with @KarishmaGhiya to make sure I understand how I can fix CI. |
Based on this: Azure#7530
|
I believe this PR is good to go. CI works (management steps are bad on master too). The changes are essentially good. I'm ready for any feedback 🙌 |
|
If we need to squash and merge this PR, I have the permissions to do so. Let me know. The management plane pipeline is failing elsewhere too. @sadasant |
sadasant
merged commit 281ce65
into
Azure:hotfix/keyvault-challengeAuth-certificates-from-4.0.0
sadasant
deleted the
hotfix/keyvault-challengeAuth-certificates-from-4.0.0-other-upgrades
branch
3 participants
Changes from: https://github.com/Azure/azure-sdk-for-js/pull/7998/files Plus a bunch of other things I had to do.
The upgrade to identity's 1.1.0-preview.3 was suggested by Jeff Fisher, and it worked nicely!
CI can possibly fail still, so beware of that. I'll make sure it passes though! Just have patience.