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 #34738

Merged
merged 2 commits into from
Oct 10, 2023

Conversation

pankajastro
Copy link
Member

@pankajastro pankajastro commented Oct 3, 2023

Related to: #30199


^ 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.

@@ -90,7 +89,6 @@ class AzureContainerInstancesOperator(BaseOperator):
:param restart_policy: Restart policy for all containers within the container group.
Possible values include: 'Always', 'OnFailure', 'Never'
:param ip_address: The IP address type of the container group.
:param network_profile: The network profile information for a container group.
Copy link
Contributor

Choose a reason for hiding this comment

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

This is breaking change.
Please set major version on provider.yaml we will also need entry in changelog.rst (top of the file) with the things that were broken and how to mitigate them.

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, still try a few things, will update the provider.yaml and changelog.rst if needed

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah, updated provider.yaml and changelog

@strictness
Copy link

Hi, I'm just adding some additional info for you guys that will be relevant once you come to updating the hooks (related issue #34749)

The hook in container_instance.py should use the azure.identity package instead of azure.common.credentials. Link to code

Microsoft posted deprecation warnings about using that package since early 2021 - azure.common.

Azure/azure-sdk-for-python#16908 (comment), where the solution is to start using azure.identity instead.

@@ -19,7 +19,7 @@

import warnings
from functools import cached_property
from typing import TYPE_CHECKING, Any
from typing import TYPE_CHECKING, Any, cast

from azure.common.client_factory import get_client_from_auth_file, get_client_from_json_dict
from azure.common.credentials import ServicePrincipalCredentials

Choose a reason for hiding this comment

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

Package is deprecated and no longer working. Consider using azure.identity (relevant info)

Related issue on azure-sdk-for-python

Copy link
Member Author

Choose a reason for hiding this comment

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

Removed ServicePrincipalCredentials and added ClientSecretCredential from azure.identity

@pankajastro pankajastro force-pushed the bump-azure-mgmt-containerinstance branch 2 times, most recently from f35f605 to 39eb6e4 Compare October 4, 2023 18:36
@pankajastro pankajastro marked this pull request as ready for review October 5, 2023 06:15
@@ -52,7 +52,10 @@ def setup_test_cases(self, create_mock_connection):
conn_type="azure_container_instances",
login="login",
password="key",
extra={"tenantId": "tenant_id", "subscriptionId": "subscription_id"},
extra={
"tenantId": "63e85d06-62e4-11ee-8c99-0242ac120002",
Copy link
Member

Choose a reason for hiding this comment

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

Is it a valid id? or just some random one?

Copy link
Member Author

Choose a reason for hiding this comment

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

it is Random. I use https://www.uuidgenerator.net/ to generate it. The new sdk throws a value error if tenantId is not valid uuid


* Remove ``network_profile`` param from ``AzureContainerInstancesOperator``
* Remove deprecated ``extra__azure__tenantId`` from azure_container_instance connection extras
* Remove deprecated ``extra__azure__subscriptionId`` from azure_container_instance connection extras
Copy link
Member

Choose a reason for hiding this comment

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

Should we mention the named tuple Volume has been removed as well?

Copy link
Member Author

Choose a reason for hiding this comment

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

I have reverted this PTAL

@pankajastro pankajastro force-pushed the bump-azure-mgmt-containerinstance branch from 39eb6e4 to 7f08469 Compare October 6, 2023 07:58
@pankajastro pankajastro force-pushed the bump-azure-mgmt-containerinstance branch from 7f08469 to e3d6079 Compare October 6, 2023 08:02
@pankajastro pankajastro merged commit 9ee14a0 into apache:main Oct 10, 2023
65 checks passed
@pankajastro pankajastro deleted the bump-azure-mgmt-containerinstance branch October 10, 2023 07:27
potiuk pushed a commit that referenced this pull request Oct 29, 2023
* Bump azure-mgmt-containerinstance

* Apply review suggestions

(cherry picked from commit 9ee14a0)
potiuk pushed a commit that referenced this pull request Oct 29, 2023
* Bump azure-mgmt-containerinstance

* Apply review suggestions

(cherry picked from commit 9ee14a0)
@potiuk potiuk added this to the Airflow 2.7.3 milestone Oct 29, 2023
@potiuk potiuk added the type:misc/internal Changelog: Misc changes that should appear in change log label Oct 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:providers provider:microsoft-azure Azure-related issues type:misc/internal Changelog: Misc changes that should appear in change log
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants