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.

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

[azure-security-keyvault-*] Add new ports #17143

Conversation

azure-sdk
Copy link
Contributor

Update vcpkg ports for Azure SDK release. This release may contain multiple ports.

## 4.0.0-beta.1 (2021-04-07)

### New Features

- KeyVaultException.
## 4.0.0-beta.1 (2021-04-07)

### New Features

- Added `Azure::Security::KeyVault::Keys::KeyClient` for get, create, list, delete, backup, restore, and import key operations.
- Added high-level and simplified `key_vault.hpp` file for simpler include experience for customers.
- Added model types which are returned from the `KeyClient` operations, such as `Azure::Security::KeyVault::Keys::KeyVaultKey`.
@ahsonkhan
Copy link
Member

This new port needs #17142 to go in first since it depends on azure-core-cpp.

@autoantwort
Copy link
Contributor

So this should be merged into vcpkg and is not spam?

@autoantwort
Copy link
Contributor

From here:

Many existing ports use the CONTROL file syntax; while this syntax will be supported for some time to come, new ports should not use these. Any newly added port must use the manifest files.

You can use vcpkg format-manifest ports/azure-security-keyvault-common-cpp/CONTROL ports/azure-security-keyvault-keys-cpp/CONTROL to convert the files

@NancyLi1013 NancyLi1013 added category:new-port The issue is requesting a new library to be added; consider making a PR! depends:different-pr This PR or Issue depends on a PR which has been filed labels Apr 8, 2021
Build-Depends: azure-core-cpp
Description: Microsoft Azure Common Key Vault SDK for C++
This library provides common Azure KeyVault-related abstractions for Azure SDK.
Homepage: https://github.com/Azure/azure-sdk-for-cpp/tree/master/sdk/keyvault/azure-security-keyvault-common
Copy link
Contributor

Choose a reason for hiding this comment

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

As suggested from @autoantwort #17143 (comment), we prefer to use vcpkg.json instead of CONTROL file for new ports. So could you please update this?

Copy link
Member

Choose a reason for hiding this comment

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

@antkmsft and @danieljurek, we need to update our package definition and publish automation process to use the new mechanism. Can you please take a look.

Copy link
Member

Choose a reason for hiding this comment

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

@NancyLi1013 we can't do the update right now. We just invested time into this and wrote automation and integration with our CI/CD a couple of months ago, when the CONTROL file was mainstream. We don't write these files manually. We'll need to upgrade our whole repo, we practically can't do this on an one-off basis by hand. It does not mean we are refusing the upgrade - we'll likely look into this, but if it is not something you mandate, please don't block us. We want to have several beta releases of our SDK in VcPkg to collect customer feedback and smoothen some stories, including VcPkg consumption.

OUT_SOURCE_PATH SOURCE_PATH
REPO Azure/azure-sdk-for-cpp
REF azure-security-keyvault-common_4.0.0-beta.1
SHA512 7364bf775bbf6115bf54ee0c2df8b2bae35ea31c669fcbc358edd00a948f2c73926fbf88282ff5442f40ead9cdfc78661a08e4f86002ac2eac5e78ddd13eb33d
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
SHA512 7364bf775bbf6115bf54ee0c2df8b2bae35ea31c669fcbc358edd00a948f2c73926fbf88282ff5442f40ead9cdfc78661a08e4f86002ac2eac5e78ddd13eb33d
SHA512 7364bf775bbf6115bf54ee0c2df8b2bae35ea31c669fcbc358edd00a948f2c73926fbf88282ff5442f40ead9cdfc78661a08e4f86002ac2eac5e78ddd13eb33d
HEAD_REF master

Please see the reference here https://github.com/microsoft/vcpkg/blob/master/docs/maintainers/vcpkg_from_github.md#notes

Build-Depends: azure-security-keyvault-common-cpp
Description: Microsoft Azure Key Vault Keys SDK for C++
This library provides Azure Key Vault Keys SDK.
Homepage: https://github.com/Azure/azure-sdk-for-cpp/tree/master/sdk/keyvault/azure-security-keyvault-keys
Copy link
Contributor

Choose a reason for hiding this comment

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

The same as above. vcpkg.json is more preferable for new ports.

Copy link
Member

Choose a reason for hiding this comment

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

This has been addressed.

OUT_SOURCE_PATH SOURCE_PATH
REPO Azure/azure-sdk-for-cpp
REF azure-security-keyvault-keys_4.0.0-beta.1
SHA512 fb5158320c859f27c97c86908fd4c3f7ceec2ec8b0ca3c213a075992b62dfdefa3a560173bc7034153c273c30abd7fd0a84f98842810de2f78412549e5d34ee4
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
SHA512 fb5158320c859f27c97c86908fd4c3f7ceec2ec8b0ca3c213a075992b62dfdefa3a560173bc7034153c273c30abd7fd0a84f98842810de2f78412549e5d34ee4
SHA512 fb5158320c859f27c97c86908fd4c3f7ceec2ec8b0ca3c213a075992b62dfdefa3a560173bc7034153c273c30abd7fd0a84f98842810de2f78412549e5d34ee4
HEAD_REF master

Please see https://github.com/microsoft/vcpkg/blob/master/docs/maintainers/vcpkg_from_github.md#notes

Copy link
Member

Choose a reason for hiding this comment

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

@NancyLi1013, we use REF, why do we also need HEAD_REF master? We tag our releases, we don't release from master.

Copy link
Contributor

Choose a reason for hiding this comment

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

So you only use REF, which is not related with branch. right? If yes, HEAD_REF might not be required.

Copy link
Member

Choose a reason for hiding this comment

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

Yes - we tag releases in master branch and release from there: https://github.com/Azure/azure-sdk-for-cpp/tags

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry for my wrong understanding about this. If we add HEAD_REF, will enable support for install --head.
https://github.com/microsoft/vcpkg/blob/master/docs/maintainers/vcpkg_from_github.md#head_ref

So this is an extra feature. Adding it or not are both good.

@NancyLi1013 NancyLi1013 changed the title Azure Key Vault Keys SDK Apr Release [azure-security-keyvault-*] Add new ports Apr 8, 2021
@ahsonkhan
Copy link
Member

ahsonkhan commented Apr 8, 2021

So this should be merged into vcpkg and is not spam?

Yes, this is intended to be merged. We will no longer open non-draft PRs that are just for tests, and limit even the draft PRs that are open (ideally just one - #17119).

@ahsonkhan
Copy link
Member

@autoantwort and @NancyLi1013 - are we allowed to change from the CONTROL format to the vcpkg.json format for the existing ports as well, for consistency? We have multiple packages coming from https://github.com/Azure/azure-sdk-for-cpp, some already in the registry and use the CONTROL file syntax (like azure-core-cpp) and some new (like this PR), and we want to avoid maintaining/supporting multiple formats for publishing.

@autoantwort
Copy link
Contributor

Yeah, the vcpkg.json format is always preferred, the Control files are only legacy. But you should not create PRs where you only change the format, do it when you do a normal library update :)

@ahsonkhan
Copy link
Member

FYI @RickWinter

@ras0219-msft
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@ahsonkhan
Copy link
Member

@NancyLi1013 and @autoantwort, I have addressed the feedback to use vcpkg manifest json file instead of CONTROL for this package. This PR is ready to be merged.

Thanks!

@ahsonkhan
Copy link
Member

@ras0219-msft can you please help merge this change. We'd like to get this package published today to complete our April release. Appreciate it.

@ras0219-msft ras0219-msft merged commit 414bec0 into microsoft:master Apr 17, 2021
@ahsonkhan ahsonkhan deleted the azure-keyvault-keys-sdk-for-cpp-april-2021 branch April 17, 2021 00:13
@ahsonkhan
Copy link
Member

🎉

Jimmy-Hu added a commit to Jimmy-Hu/vcpkg that referenced this pull request Apr 17, 2021
[azure-security-keyvault-*] Add new ports (microsoft#17143)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:new-port The issue is requesting a new library to be added; consider making a PR! depends:different-pr This PR or Issue depends on a PR which has been filed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants