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

[keyvault] Remove Python 3.6 and six references #26150

Closed
wants to merge 7 commits into from
Closed

[keyvault] Remove Python 3.6 and six references #26150

wants to merge 7 commits into from

Conversation

wonhyeongseo
Copy link
Contributor

@wonhyeongseo wonhyeongseo commented Sep 11, 2022 •
edited
Loading

Description

Dear @mccoyp, after review we found some changes were lost during updates.
Thank you.

Is your feature request related to a problem? Please describe.
In accordance with the August Milestone of Azure for dropping Python 3.6 support, changes that were made to the keyvault packages have been recovered. We also recovered @ponopono0322's contribution of removing references to the six package.

Describe the solution you'd like
Drop Python 3.6 support and add 3.10 to the supported versions list.
Remove all references of six package from keyvault code.

Describe alternatives you've considered
None

Additional context
#25449
#25902

All SDK Contribution checklist:

  • The pull request does not introduce [breaking changes]
  • CHANGELOG is updated for new features, bug fixes or other significant changes.
  • I have read the contribution guidelines.

General Guidelines and Best Practices

  • Title of the pull request is clear and informative.
  • There are a small number of commits, each of which have an informative message. This means that previously merged commits do not appear in the history of the PR. For more information on cleaning up the commits in your PR, see this page.

Testing Guidelines

  • Pull request includes test coverage for the included changes.

@ghost ghost added KeyVault customer-reported Issues that are reported by GitHub users external to the Azure organization. labels Sep 11, 2022
@ghost
Copy link

ghost commented Sep 11, 2022

Thank you for your contribution wonhyeongseo! We will review the pull request and get back to you soon.

mccoyp
mccoyp reviewed Sep 23, 2022
View reviewed changes
Copy link
Member

@mccoyp mccoyp left a comment

Choose a reason for hiding this comment

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

Thank you so much again for doing this! Dropping six is a lot of work, and I really appreciate it 🙂 I'm sorry about the late review on this; there have been a number of releases in KV recently and it was difficult to merge things in while that was being worked on.

Based on your PR description, it sounds like we can close #25902 and use this PR instead. Is that correct?

sdk/keyvault/azure-keyvault-administration/CHANGELOG.md Outdated Show resolved Hide resolved
@mccoyp
Copy link
Member

mccoyp commented Sep 23, 2022

By the way -- I'm currently regenerating our generated code, and that will drop the implicit six dependency and should get CI passing on this PR. I'll merge the regeneration work soon and then we can merge this right after 🙂

@mccoyp mccoyp mentioned this pull request Sep 24, 2022
Closed
6 tasks
@mccoyp mccoyp mentioned this pull request Sep 26, 2022
Closed
6 tasks
@mccoyp
Copy link
Member

mccoyp commented Nov 3, 2022

Hi again @wonhyeongseo, I'm back! The code regeneration I mentioned earlier just went through today: #27170

If you update this PR with main, this PR should be just about ready to go. There are a number of merge conflicts since this has been up for a while though (which is because of us -- my apologies), so it may be easier to open a separate PR with these changes.

One last note: the azure-mgmt-keyvault package is owned by a different set of folks than the azure-keyvault-* packages, so it would be best to leave azure-mgmt-keyvault changes for a separate PR. You can tag me if you'd like to open one, and I would make sure the package owners see it!

Thank you again for the contribution and for your patience!

@ponopono0322 ponopono0322 mentioned this pull request Nov 9, 2022
Merged
6 tasks
@wonhyeongseo
Copy link
Contributor Author

Closing in favor of #27377

@wonhyeongseo wonhyeongseo deleted the keyvault-drop3.6 branch November 13, 2022 02:10
azure-sdk pushed a commit to azure-sdk/azure-sdk-for-python that referenced this pull request Oct 20, 2023
@karataliu
CodeGen from PR 26150 in Azure/azure-rest-api-specs
22e1c2a 
[Hub Generated] Review request for Microsoft.ContainerService/aks to add version stable/2023-09-01 (Azure#26150)

* Adds base for updating Microsoft.ContainerService/aks from version stable/2023-08-01 to version 2023-09-01

* Updates readme

* Updates API version in new specs and examples

* update (Azure#26127)

* update readmes (Azure#26125)

* [AKS] Add trustedAccessRoles and trustedAccessRoleBindings into Microsoft.ContainerService 2023-09-01 api (Azure#26043)

* Add trustedAccessRoles and trustedAccessRoleBindings into Microsoft.ContainerService 2023-09-01 api

* fixup! Add trustedAccessRoles and trustedAccessRoleBindings into Microsoft.ContainerService 2023-09-01 api

* update common types from v3 to v5 (Azure#26214)

---------

Co-authored-by: Dong Liu <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mccoyp mccoyp mccoyp left review comments

@schaabs schaabs Awaiting requested review from schaabs

@YalinLi0312 YalinLi0312 Awaiting requested review from YalinLi0312

@BigCat20196 BigCat20196 Awaiting requested review from BigCat20196

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

Assignees
No one assigned
Labels
customer-reported Issues that are reported by GitHub users external to the Azure organization. KeyVault
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@wonhyeongseo @mccoyp