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

Bump azure-mgmt-containerinstance>=7.0.0,<9.0.0 #33696

Merged
merged 4 commits into from
Aug 28, 2023

Conversation

pankajastro
Copy link
Member


^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named {pr_number}.significant.rst or {issue_number}.significant.rst, in newsfragments.

@potiuk
Copy link
Member

potiuk commented Aug 24, 2023

No dice :( ... need some work I am afraid.

@pankajastro
Copy link
Member Author

No dice :( ... need some work I am afraid.

Yeah, will investigate it and let see how it goes

@eladkal
Copy link
Contributor

eladkal commented Aug 24, 2023

One of the issues is with network_profile:

network_profile: ContainerGroupNetworkProfile | None = None,

The parameter (type ContainerGroupNetworkProfile )was removed in version 9.0.0
https://github.com/Azure/azure-sdk-for-python/blob/main/sdk/containerinstance/azure-mgmt-containerinstance/CHANGELOG.md#900-2021-09-17

From official docs:

Network profiles have been retired as of the 2021-07-01 API version. If you're using this or a more recent version, ignore any steps and actions related to network profiles.

https://learn.microsoft.com/en-us/azure/container-instances/container-instances-virtual-network-concepts#network-profile

Thus bumping azure-mgmt-containerinstance to version greater than 9.0.0 means also breaking change for the provider (Which we should do!)

@pankajastro
Copy link
Member Author

Thanks @eladkal I want azure-identity which is supported in 7.0.0>= but I feel should be ok to change to 9.0.0

@eladkal
Copy link
Contributor

eladkal commented Aug 24, 2023

Thanks @eladkal I want azure-identity which is supported in 7.0.0>= but I feel should be ok to change to 9.0.0

azure-mgmt-containerinstance>=7.0.0 is not going to work because of the network_profile issue.
so you have two options:

  1. fix all issues and have azure-mgmt-containerinstance>=9.0.0
  2. Set azure-mgmt-containerinstance>=7.0.0, <9.0.0

in the 2nd option you won't need to deal with network_profile now

@pankajastro pankajastro force-pushed the bump-azure-mgmt-containerinstance branch from ce04108 to de0ce51 Compare August 25, 2023 19:34
@pankajastro pankajastro force-pushed the bump-azure-mgmt-containerinstance branch from de0ce51 to 5f2c6f3 Compare August 25, 2023 19:35
@eladkal eladkal changed the title Bump azure-mgmt-containerinstance>=7.0.0 Bump azure-mgmt-containerinstance>=7.0.0,<9.0.0 Aug 27, 2023
@potiuk
Copy link
Member

potiuk commented Aug 27, 2023

Looks like at least tests are green @pankajastro :)

@pankajastro pankajastro force-pushed the bump-azure-mgmt-containerinstance branch from 35bdfa2 to 04f92b0 Compare August 28, 2023 08:23
@eladkal
Copy link
Contributor

eladkal commented Aug 28, 2023

@pankajastro is it ready for review and merge?

@pankajastro pankajastro marked this pull request as ready for review August 28, 2023 11:02
@pankajastro
Copy link
Member Author

@pankajastro is it ready for review and merge?

yes, just mark it ready for review. I have done with testing and worked

@potiuk potiuk merged commit 9d53278 into apache:main Aug 28, 2023
64 checks passed
@potiuk
Copy link
Member

potiuk commented Aug 28, 2023

Cooool.... One less thing to bump for Azure :). I am not sure if they deserve it, but well ... Who am I to judge.

@eladkal
Copy link
Contributor

eladkal commented Aug 28, 2023

We still need to bump for >=9.0.0 and this means handling the issues I listed in #33696 (comment)

@pankajastro
Copy link
Member Author

We still need to bump for >=9.0.0 and this means handling the issues I listed in #33696 (comment)

Yes, I'll investigate it later this week

@potiuk
Copy link
Member

potiuk commented Aug 28, 2023

I also updated #30199

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants