Skip to content

Conversation

@sadasant
Copy link
Contributor

@sadasant sadasant commented May 7, 2020

This PR has what is needed to make the base branch for the challenge based authentication hotfix stable.

Related to:

Copy link
Contributor

Choose a reason for hiding this comment

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

What is the reason for this casting? I understand that jws.decode() takes a string, but why is it that we get an error now, but not when the last stable version of keyvault keys was released?

Copy link
Contributor Author

@sadasant sadasant May 8, 2020

Choose a reason for hiding this comment

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

I don't know. I know that as we go back in time, stability on the commits to master is lost, due to how the environment changes. By environment I mean all of the projects we build, but even more how the CI pipelines have changed. Only recently we've been enforcing stuff more consistently across the whole project.

During last week's hotfix release for identity, there were similar issues. Not similar to this specific change, but core-lro reported to be broken on a commit that should have been green, since the commit was used to release another package. When I investigated the issue, I was able to confirm that many commits in the past, despite having a relationship with releases, either appeared to be broken on the github reports, or were broken locally when I tried to build them. I also found out that the CI logs are lost after some time in the past. I reported all of this to @KarishmaGhiya

There's also the fact that this change does absolutely no damage to the keyvault hotfix release.

Copy link
Contributor Author

@sadasant sadasant May 8, 2020

Choose a reason for hiding this comment

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

Just to avoid loose threads: The way core-lro was fixed for the identity release was to move it forward to the next stable release, only in that folder, while downgrading a couple of dependencies. Fortunately, identity didn't depend on core-lro, so that was fine.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, but as string should work just fine right? Why do we need to cast to any and then cast to string?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just as string was enough indeed! Nice catch!

@ramya-rao-a
Copy link
Contributor

@KarishmaGhiya Can you review the pipeline related changes in this PR?

@ramya-rao-a
Copy link
Contributor

Thanks @KarishmaGhiya

@sadasant Please merge this PR and update the 3 related PRs

@sadasant
Copy link
Contributor Author

sadasant commented May 9, 2020

I can merge this PR.

@sadasant sadasant merged commit f08ac0f into Azure:hotfix/keyvault-challengeAuth-keys-secrets-4.0.3 May 9, 2020
@sadasant sadasant deleted the hotfix/keyvault-challengeAuth-keys-secrets-4.0.3-CI-fix branch May 9, 2020 11:44
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.

5 participants