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

Migrated Key Vault JCA library to use azure-json serialization/deserialization #41508

Merged
merged 15 commits into from
Oct 30, 2024
Merged

Migrated Key Vault JCA library to use azure-json serialization/deserialization #41508

merged 15 commits into from
Oct 30, 2024

Conversation

vcolin7
Copy link
Member

@vcolin7 vcolin7 commented Aug 14, 2024

This library shades Jackson classes to avoid having many external dependencies, so I've gone ahead and done the same for azure-json. I did copy a number of classes over from azure-core, so maybe there's a better approach for that. Let me know what you think @alzimmermsft.

For more information on this, see: #40614.

@vcolin7 vcolin7 added KeyVault Client This issue points to a problem in the data-plane of the library. azure-spring-jca labels Aug 14, 2024
@vcolin7 vcolin7 requested a review from alzimmermsft August 14, 2024 05:57
@vcolin7 vcolin7 self-assigned this Aug 14, 2024
alzimmermsft
alzimmermsft requested changes Aug 14, 2024
View reviewed changes
Copy link
Member

@alzimmermsft alzimmermsft left a comment

Choose a reason for hiding this comment

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

With the recommended changes to the JSON utilities class, you can delete out all the code copied from azure-core

...ca/src/main/java/com/azure/security/keyvault/jca/implementation/utils/JsonConverterUtil.java Outdated Show resolved Hide resolved
...eyvault-jca/src/main/java/com/azure/security/keyvault/jca/implementation/KeyVaultClient.java Outdated Show resolved Hide resolved
...ault-jca/src/main/java/com/azure/security/keyvault/jca/implementation/model/AccessToken.java Outdated Show resolved Hide resolved
...rc/main/java/com/azure/security/keyvault/jca/implementation/model/CertificateListResult.java Outdated Show resolved Hide resolved
...rc/main/java/com/azure/security/keyvault/jca/implementation/model/CertificateListResult.java Show resolved Hide resolved
...lt-jca/src/main/java/com/azure/security/keyvault/jca/implementation/model/KeyProperties.java Outdated Show resolved Hide resolved
...vault-jca/src/main/java/com/azure/security/keyvault/jca/implementation/model/SignResult.java Show resolved Hide resolved
@saragluna
Copy link
Member

Hey @vcolin7, thanks for creating this PR. I am wondering whether we should revert the two changes you made for the AAD Login URL, before we merge in any PR? Seems like you are actively contributing to make the JCA library better, and we also have some features that want to be merged in, and out as regular release. So I think if those two changes won't be out as GA in the near future, maybe we should revert it, and merge it back to main when it's ready to GA.

@vcolin7
Copy link
Member Author

vcolin7 commented Aug 19, 2024

Hey @vcolin7, thanks for creating this PR. I am wondering whether we should revert the two changes you made for the AAD Login URL, before we merge in any PR? Seems like you are actively contributing to make the JCA library better, and we also have some features that want to be merged in, and out as regular release. So I think if those two changes won't be out as GA in the near future, maybe we should revert it, and merge it back to main when it's ready to GA.

I think if you guys want to release a single feature that's already in main we should not revert the commits, but instead create a release branch containing only said feature, which is what we do with our other data plane libraries. We have a script that can help you set up the branch and then it would be a matter of cherry-picking the commits that include the feature work. We can talk about this on a separate chat if you need any assistance setting things up.

@vcolin7 vcolin7 force-pushed the feature/vicolina/keyvault/jca/azure-json-migration branch from b8b3774 to 2aa3fd6 Compare August 19, 2024 18:27
@saragluna
Copy link
Member

Thanks @vcolin7. But what I'm talking is we are having a PR #41303, we would like to merge into the main branch. But the main branch already has changes only suitable for beta releases. So my question is, should we use the main branch as a place for code ready for GA release, and move the code for beta releases to a feature branch?

@azure-sdk
Copy link
Collaborator

API change check

API changes are not detected in this pull request.

@vcolin7
Copy link
Member Author

vcolin7 commented Sep 4, 2024

Thanks @vcolin7. But what I'm talking is we are having a PR #41303, we would like to merge into the main branch. But the main branch already has changes only suitable for beta releases. So my question is, should we use the main branch as a place for code ready for GA release, and move the code for beta releases to a feature branch?

I think we should merge your PR to main and then release a new stable version from a separate release branch that does not include the beta changes we are not ready to GA yet. This is because if we release any other beta in the future, it should not be missing features or bug fixes we already released for a "previous" stable version (like this that would be included in a 2.8.2 release). Reverting the commits with the beta work would add unnecessary overhead. We should be getting close to GA'ing the beta work anyway, we're just waiting for customer feedback on it.

@joshfree
Copy link
Member

@vcolin7 what's happening with this PR? Can we get this wrapped up?

@vcolin7
Merge branch 'main' into feature/vicolina/keyvault/jca/azure-json-mig…
4f91ad8 
…ration

# Conflicts:
#	sdk/keyvault/azure-security-keyvault-jca/src/main/java/com/azure/security/keyvault/jca/implementation/KeyVaultClient.java
#	sdk/keyvault/azure-security-keyvault-jca/src/main/java/com/azure/security/keyvault/jca/implementation/model/SecretBundle.java
#	sdk/keyvault/azure-security-keyvault-jca/src/main/java/com/azure/security/keyvault/jca/implementation/utils/AccessTokenUtil.java
#	sdk/keyvault/azure-security-keyvault-jca/src/main/java/com/azure/security/keyvault/jca/implementation/utils/JsonConverterUtil.java
@vcolin7
Applied Spotless.
c3e970c
@vcolin7 vcolin7 enabled auto-merge (squash) October 25, 2024 18:20
@vcolin7 vcolin7 disabled auto-merge October 30, 2024 18:45
@vcolin7
Copy link
Member Author

vcolin7 commented Oct 30, 2024

/azp run java - keyvault - ci

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@vcolin7 vcolin7 merged commit b088168 into main Oct 30, 2024
19 checks passed
@vcolin7 vcolin7 deleted the feature/vicolina/keyvault/jca/azure-json-migration branch October 30, 2024 21:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alzimmermsft alzimmermsft alzimmermsft approved these changes

@rujche rujche Awaiting requested review from rujche rujche is a code owner

@Netyyyy Netyyyy Awaiting requested review from Netyyyy Netyyyy is a code owner

@saragluna saragluna Awaiting requested review from saragluna saragluna is a code owner

@moarychan moarychan Awaiting requested review from moarychan moarychan is a code owner

Assignees

@vcolin7 vcolin7

Labels
azure-spring-jca Client This issue points to a problem in the data-plane of the library. KeyVault
Projects
Azure SDK for Key Vault
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@vcolin7 @saragluna @azure-sdk @joshfree @alzimmermsft