You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by GitBox <gi...@apache.org> on 2022/12/09 23:44:42 UTC
[GitHub] [airflow] Taragolis opened a new pull request, #28279: Add Amazon Elastic Container Registry (ECR) Hook
Taragolis opened a new pull request, #28279:
URL: https://github.com/apache/airflow/pull/28279
Pre-requirements for https://github.com/apache/airflow/pull/26162
Add new Hook into amazon-provider which integrated with ECR.
Hook itself it is just Thin wrapper around boto3 ECR client with single method for:
- Obtain temporary credentials
- Decode them
- Mask password
- Prepare to use with `docker login`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a diff in pull request #28279: Add Amazon Elastic Container Registry (ECR) Hook
Posted by GitBox <gi...@apache.org>.
potiuk commented on code in PR #28279:
URL: https://github.com/apache/airflow/pull/28279#discussion_r1045031787
##########
airflow/providers/amazon/aws/hooks/ecr.py:
##########
@@ -0,0 +1,100 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+from __future__ import annotations
+
+import base64
+from dataclasses import dataclass
+from datetime import datetime
+
+from airflow.compat.functools import cached_property
+from airflow.providers.amazon.aws.hooks.base_aws import AwsBaseHook
+from airflow.utils.log.logging_mixin import LoggingMixin
+from airflow.utils.log.secrets_masker import mask_secret
+
+
+@dataclass
+class EcrCredentials(LoggingMixin):
+ """Helper class for store temporary ECR credentials."""
+
+ username: str
+ password: str
+ proxy_endpoint: str
+ expires_at: datetime
+
+ def __post_init__(self):
+ mask_secret(self.password)
+ self.log.debug("Credentials to Amazon ECR %r expires at %s.", self.proxy_endpoint, self.expires_at)
+
+ @cached_property
+ def registry(self) -> str:
+ """Return registry in appropriate `docker login` format."""
+ # https://github.com/docker/docker-py/issues/2256#issuecomment-824940506
+ return self.proxy_endpoint.replace("https://", "")
Review Comment:
proxy_endpoint is not immutable. Are you sure cached_property is a good idea ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ferruzzi commented on a diff in pull request #28279: Add Amazon Elastic Container Registry (ECR) Hook
Posted by GitBox <gi...@apache.org>.
ferruzzi commented on code in PR #28279:
URL: https://github.com/apache/airflow/pull/28279#discussion_r1047744952
##########
tests/providers/amazon/aws/hooks/test_ecr.py:
##########
@@ -0,0 +1,103 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+from __future__ import annotations
+
+from unittest import mock
+
+import boto3
+import pytest
+from moto import mock_ecr
+from moto.core import DEFAULT_ACCOUNT_ID
+
+from airflow.providers.amazon.aws.hooks.ecr import EcrHook
+
+
+@pytest.fixture
+def patch_hook(monkeypatch):
+ """Patch hook object by dummy boto3 ECR client."""
+ ecr_client = boto3.client("ecr")
+ monkeypatch.setattr(EcrHook, "conn", ecr_client)
+ yield
+
+
+@mock_ecr
+class TestEcrHook:
+ def test_service_type(self):
+ """Test expected boto3 client type."""
+ assert EcrHook().client_type == "ecr"
+
+ @pytest.mark.parametrize(
+ "accounts_ids",
+ [
+ pytest.param("", id="empty-string"),
+ pytest.param(None, id="none"),
+ pytest.param([], id="empty-list"),
+ ],
+ )
+ def test_get_temporary_credentials_default_account_id(self, patch_hook, accounts_ids):
+ """Test different types of empty account/registry ids."""
+ result = EcrHook().get_temporary_credentials(registry_ids=accounts_ids)
+ assert len(result) == 1
+ assert result[0].username == "AWS"
+ assert result[0].registry.startswith(DEFAULT_ACCOUNT_ID)
+ assert result[0].password == f"{DEFAULT_ACCOUNT_ID}-auth-token"
+
+ @pytest.mark.parametrize(
+ "accounts_id, expected_registry",
+ [
+ pytest.param(DEFAULT_ACCOUNT_ID, DEFAULT_ACCOUNT_ID, id="moto-default-account"),
+ pytest.param("111100002222", "111100002222", id="custom-account-id"),
+ pytest.param(["333366669999"], "333366669999", id="empty-list"),
+ ],
+ )
+ def test_get_temporary_credentials_single_account_id(self, patch_hook, accounts_id, expected_registry):
Review Comment:
Sure, it was just a thought. :+1:
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] Taragolis commented on a diff in pull request #28279: Add Amazon Elastic Container Registry (ECR) Hook
Posted by GitBox <gi...@apache.org>.
Taragolis commented on code in PR #28279:
URL: https://github.com/apache/airflow/pull/28279#discussion_r1045040610
##########
airflow/providers/amazon/aws/hooks/ecr.py:
##########
@@ -0,0 +1,100 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+from __future__ import annotations
+
+import base64
+from dataclasses import dataclass
+from datetime import datetime
+
+from airflow.compat.functools import cached_property
+from airflow.providers.amazon.aws.hooks.base_aws import AwsBaseHook
+from airflow.utils.log.logging_mixin import LoggingMixin
+from airflow.utils.log.secrets_masker import mask_secret
+
+
+@dataclass
+class EcrCredentials(LoggingMixin):
+ """Helper class for store temporary ECR credentials."""
+
+ username: str
+ password: str
+ proxy_endpoint: str
+ expires_at: datetime
+
+ def __post_init__(self):
+ mask_secret(self.password)
+ self.log.debug("Credentials to Amazon ECR %r expires at %s.", self.proxy_endpoint, self.expires_at)
+
+ @cached_property
+ def registry(self) -> str:
+ """Return registry in appropriate `docker login` format."""
+ # https://github.com/docker/docker-py/issues/2256#issuecomment-824940506
+ return self.proxy_endpoint.replace("https://", "")
Review Comment:
And also there is no point to make it cacheable - no heavy computing here
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ferruzzi commented on a diff in pull request #28279: Add Amazon Elastic Container Registry (ECR) Hook
Posted by GitBox <gi...@apache.org>.
ferruzzi commented on code in PR #28279:
URL: https://github.com/apache/airflow/pull/28279#discussion_r1047747492
##########
tests/providers/amazon/aws/hooks/test_ecr.py:
##########
@@ -0,0 +1,103 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+from __future__ import annotations
+
+from unittest import mock
+
+import boto3
+import pytest
+from moto import mock_ecr
+from moto.core import DEFAULT_ACCOUNT_ID
+
+from airflow.providers.amazon.aws.hooks.ecr import EcrHook
+
+
+@pytest.fixture
+def patch_hook(monkeypatch):
+ """Patch hook object by dummy boto3 ECR client."""
+ ecr_client = boto3.client("ecr")
+ monkeypatch.setattr(EcrHook, "conn", ecr_client)
+ yield
+
+
+@mock_ecr
+class TestEcrHook:
+ def test_service_type(self):
+ """Test expected boto3 client type."""
+ assert EcrHook().client_type == "ecr"
+
+ @pytest.mark.parametrize(
+ "accounts_ids",
+ [
+ pytest.param("", id="empty-string"),
+ pytest.param(None, id="none"),
+ pytest.param([], id="empty-list"),
+ ],
+ )
Review Comment:
Interesting. Pretty sure the second block of terminal output is a mistaken repost of the first output, but I get what you are saying. I don't think I've ever seen that used. :+1:
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] Taragolis commented on a diff in pull request #28279: Add Amazon Elastic Container Registry (ECR) Hook
Posted by GitBox <gi...@apache.org>.
Taragolis commented on code in PR #28279:
URL: https://github.com/apache/airflow/pull/28279#discussion_r1047525571
##########
airflow/providers/amazon/aws/hooks/ecr.py:
##########
@@ -0,0 +1,101 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+from __future__ import annotations
+
+import base64
+import logging
+from dataclasses import dataclass
+from datetime import datetime
+
+from airflow.providers.amazon.aws.hooks.base_aws import AwsBaseHook
+from airflow.utils.log.secrets_masker import mask_secret
+
+logger = logging.getLogger(__name__)
+
+
+@dataclass(frozen=True)
+class EcrCredentials:
+ """Helper (frozen dataclass) for storing temporary ECR credentials."""
+
+ username: str
+ password: str
+ proxy_endpoint: str
+ expires_at: datetime
+
+ def __post_init__(self):
+ mask_secret(self.password)
+ logger.debug("Credentials to Amazon ECR %r expires at %s.", self.proxy_endpoint, self.expires_at)
+
+ @property
+ def registry(self) -> str:
+ """Return registry in appropriate `docker login` format."""
+ # https://github.com/docker/docker-py/issues/2256#issuecomment-824940506
+ return self.proxy_endpoint.replace("https://", "")
+
+
+class EcrHook(AwsBaseHook):
+ """
+ Interact with Amazon Elastic Container Registry (ECR)
+
+ Additional arguments (such as ``aws_conn_id``) may be specified and
+ are passed down to the underlying AwsBaseHook.
+
+ .. seealso::
+ :class:`~airflow.providers.amazon.aws.hooks.base_aws.AwsBaseHook`
+ """
+
+ def __init__(self, **kwargs):
+ kwargs.pop("client_type", None)
+ super().__init__(client_type="ecr", **kwargs)
+
+ def get_temporary_credentials(self, registry_ids: list[str] | str | None = None) -> list[EcrCredentials]:
+ """Get temporary credentials for Amazon ECR.
+
+ Return list of :class:`~airflow.providers.amazon.aws.hooks.ecr.EcrCredentials`,
+ obtained credentials valid for 12 hours.
+
+ :param registry_ids: Either AWS Account ID or list of AWS Account IDs that are associated
+ with the registries for which obtain credentials. If you do not specify a registry,
+ the default registry is assumed.
+
+ .. seealso::
+ - `boto3 ECR client get_authorization_token method <https://boto3.amazonaws.com/v1/documentation/\
+ api/latest/reference/services/ecr.html#ECR.Client.get_authorization_token>`_.
+ """
+ registry_ids = registry_ids or None
Review Comment:
So value in some cases might be `""` just for avoid this situation.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] o-nikolas commented on a diff in pull request #28279: Add Amazon Elastic Container Registry (ECR) Hook
Posted by GitBox <gi...@apache.org>.
o-nikolas commented on code in PR #28279:
URL: https://github.com/apache/airflow/pull/28279#discussion_r1047488028
##########
airflow/providers/amazon/aws/hooks/ecr.py:
##########
@@ -0,0 +1,101 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+from __future__ import annotations
+
+import base64
+import logging
+from dataclasses import dataclass
+from datetime import datetime
+
+from airflow.providers.amazon.aws.hooks.base_aws import AwsBaseHook
+from airflow.utils.log.secrets_masker import mask_secret
+
+logger = logging.getLogger(__name__)
+
+
+@dataclass(frozen=True)
+class EcrCredentials:
+ """Helper (frozen dataclass) for storing temporary ECR credentials."""
+
+ username: str
+ password: str
+ proxy_endpoint: str
+ expires_at: datetime
+
+ def __post_init__(self):
+ mask_secret(self.password)
+ logger.debug("Credentials to Amazon ECR %r expires at %s.", self.proxy_endpoint, self.expires_at)
+
+ @property
+ def registry(self) -> str:
+ """Return registry in appropriate `docker login` format."""
+ # https://github.com/docker/docker-py/issues/2256#issuecomment-824940506
+ return self.proxy_endpoint.replace("https://", "")
+
+
+class EcrHook(AwsBaseHook):
+ """
+ Interact with Amazon Elastic Container Registry (ECR)
+
+ Additional arguments (such as ``aws_conn_id``) may be specified and
+ are passed down to the underlying AwsBaseHook.
+
+ .. seealso::
+ :class:`~airflow.providers.amazon.aws.hooks.base_aws.AwsBaseHook`
+ """
+
+ def __init__(self, **kwargs):
+ kwargs.pop("client_type", None)
+ super().__init__(client_type="ecr", **kwargs)
+
+ def get_temporary_credentials(self, registry_ids: list[str] | str | None = None) -> list[EcrCredentials]:
+ """Get temporary credentials for Amazon ECR.
+
+ Return list of :class:`~airflow.providers.amazon.aws.hooks.ecr.EcrCredentials`,
+ obtained credentials valid for 12 hours.
+
+ :param registry_ids: Either AWS Account ID or list of AWS Account IDs that are associated
+ with the registries for which obtain credentials. If you do not specify a registry,
+ the default registry is assumed.
+
+ .. seealso::
+ - `boto3 ECR client get_authorization_token method <https://boto3.amazonaws.com/v1/documentation/\
+ api/latest/reference/services/ecr.html#ECR.Client.get_authorization_token>`_.
+ """
+ registry_ids = registry_ids or None
Review Comment:
Is this just to guard empty string/list as an input? Because you are already defaulting this to None in the method signature.
##########
airflow/providers/amazon/aws/hooks/ecr.py:
##########
@@ -0,0 +1,101 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+from __future__ import annotations
+
+import base64
+import logging
+from dataclasses import dataclass
+from datetime import datetime
+
+from airflow.providers.amazon.aws.hooks.base_aws import AwsBaseHook
+from airflow.utils.log.secrets_masker import mask_secret
+
+logger = logging.getLogger(__name__)
+
+
+@dataclass(frozen=True)
+class EcrCredentials:
+ """Helper (frozen dataclass) for storing temporary ECR credentials."""
+
+ username: str
+ password: str
+ proxy_endpoint: str
+ expires_at: datetime
+
+ def __post_init__(self):
+ mask_secret(self.password)
+ logger.debug("Credentials to Amazon ECR %r expires at %s.", self.proxy_endpoint, self.expires_at)
+
+ @property
+ def registry(self) -> str:
+ """Return registry in appropriate `docker login` format."""
+ # https://github.com/docker/docker-py/issues/2256#issuecomment-824940506
+ return self.proxy_endpoint.replace("https://", "")
+
+
+class EcrHook(AwsBaseHook):
+ """
+ Interact with Amazon Elastic Container Registry (ECR)
+
+ Additional arguments (such as ``aws_conn_id``) may be specified and
+ are passed down to the underlying AwsBaseHook.
+
+ .. seealso::
+ :class:`~airflow.providers.amazon.aws.hooks.base_aws.AwsBaseHook`
+ """
+
+ def __init__(self, **kwargs):
+ kwargs.pop("client_type", None)
+ super().__init__(client_type="ecr", **kwargs)
+
+ def get_temporary_credentials(self, registry_ids: list[str] | str | None = None) -> list[EcrCredentials]:
+ """Get temporary credentials for Amazon ECR.
+
+ Return list of :class:`~airflow.providers.amazon.aws.hooks.ecr.EcrCredentials`,
+ obtained credentials valid for 12 hours.
+
+ :param registry_ids: Either AWS Account ID or list of AWS Account IDs that are associated
+ with the registries for which obtain credentials. If you do not specify a registry,
Review Comment:
```suggestion
with the registries from which credentials are obtained. If you do not specify a registry,
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] Taragolis commented on a diff in pull request #28279: Add Amazon Elastic Container Registry (ECR) Hook
Posted by GitBox <gi...@apache.org>.
Taragolis commented on code in PR #28279:
URL: https://github.com/apache/airflow/pull/28279#discussion_r1045036909
##########
airflow/providers/amazon/aws/hooks/ecr.py:
##########
@@ -0,0 +1,100 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+from __future__ import annotations
+
+import base64
+from dataclasses import dataclass
+from datetime import datetime
+
+from airflow.compat.functools import cached_property
+from airflow.providers.amazon.aws.hooks.base_aws import AwsBaseHook
+from airflow.utils.log.logging_mixin import LoggingMixin
+from airflow.utils.log.secrets_masker import mask_secret
+
+
+@dataclass
+class EcrCredentials(LoggingMixin):
+ """Helper class for store temporary ECR credentials."""
+
+ username: str
+ password: str
+ proxy_endpoint: str
+ expires_at: datetime
+
+ def __post_init__(self):
+ mask_secret(self.password)
+ self.log.debug("Credentials to Amazon ECR %r expires at %s.", self.proxy_endpoint, self.expires_at)
+
+ @cached_property
+ def registry(self) -> str:
+ """Return registry in appropriate `docker login` format."""
+ # https://github.com/docker/docker-py/issues/2256#issuecomment-824940506
+ return self.proxy_endpoint.replace("https://", "")
Review Comment:
Ooops... Yep initially I make this dataclass frozen (kind of immutable) however after I add inheritance from `LoggingMixin` so it can't be anymore frozen and forget about this property.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ferruzzi commented on a diff in pull request #28279: Add Amazon Elastic Container Registry (ECR) Hook
Posted by GitBox <gi...@apache.org>.
ferruzzi commented on code in PR #28279:
URL: https://github.com/apache/airflow/pull/28279#discussion_r1047572324
##########
tests/providers/amazon/aws/hooks/test_ecr.py:
##########
@@ -0,0 +1,103 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+from __future__ import annotations
+
+from unittest import mock
+
+import boto3
+import pytest
+from moto import mock_ecr
+from moto.core import DEFAULT_ACCOUNT_ID
+
+from airflow.providers.amazon.aws.hooks.ecr import EcrHook
+
+
+@pytest.fixture
+def patch_hook(monkeypatch):
+ """Patch hook object by dummy boto3 ECR client."""
+ ecr_client = boto3.client("ecr")
+ monkeypatch.setattr(EcrHook, "conn", ecr_client)
+ yield
+
+
+@mock_ecr
+class TestEcrHook:
+ def test_service_type(self):
+ """Test expected boto3 client type."""
+ assert EcrHook().client_type == "ecr"
+
+ @pytest.mark.parametrize(
+ "accounts_ids",
+ [
+ pytest.param("", id="empty-string"),
+ pytest.param(None, id="none"),
+ pytest.param([], id="empty-list"),
+ ],
+ )
Review Comment:
This might be my own ignorance, but why bother with the pytest.param and setting an id? I've always done this as
`@pytest.mark.parametrize("accounts_ids", ["", None, []])`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ferruzzi commented on a diff in pull request #28279: Add Amazon Elastic Container Registry (ECR) Hook
Posted by GitBox <gi...@apache.org>.
ferruzzi commented on code in PR #28279:
URL: https://github.com/apache/airflow/pull/28279#discussion_r1047577311
##########
tests/providers/amazon/aws/hooks/test_ecr.py:
##########
@@ -0,0 +1,103 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+from __future__ import annotations
+
+from unittest import mock
+
+import boto3
+import pytest
+from moto import mock_ecr
+from moto.core import DEFAULT_ACCOUNT_ID
+
+from airflow.providers.amazon.aws.hooks.ecr import EcrHook
+
+
+@pytest.fixture
+def patch_hook(monkeypatch):
+ """Patch hook object by dummy boto3 ECR client."""
+ ecr_client = boto3.client("ecr")
+ monkeypatch.setattr(EcrHook, "conn", ecr_client)
+ yield
+
+
+@mock_ecr
+class TestEcrHook:
+ def test_service_type(self):
+ """Test expected boto3 client type."""
+ assert EcrHook().client_type == "ecr"
+
+ @pytest.mark.parametrize(
+ "accounts_ids",
+ [
+ pytest.param("", id="empty-string"),
+ pytest.param(None, id="none"),
+ pytest.param([], id="empty-list"),
+ ],
+ )
+ def test_get_temporary_credentials_default_account_id(self, patch_hook, accounts_ids):
+ """Test different types of empty account/registry ids."""
+ result = EcrHook().get_temporary_credentials(registry_ids=accounts_ids)
+ assert len(result) == 1
+ assert result[0].username == "AWS"
+ assert result[0].registry.startswith(DEFAULT_ACCOUNT_ID)
+ assert result[0].password == f"{DEFAULT_ACCOUNT_ID}-auth-token"
+
+ @pytest.mark.parametrize(
+ "accounts_id, expected_registry",
+ [
+ pytest.param(DEFAULT_ACCOUNT_ID, DEFAULT_ACCOUNT_ID, id="moto-default-account"),
+ pytest.param("111100002222", "111100002222", id="custom-account-id"),
+ pytest.param(["333366669999"], "333366669999", id="empty-list"),
+ ],
+ )
+ def test_get_temporary_credentials_single_account_id(self, patch_hook, accounts_id, expected_registry):
Review Comment:
I'd also consider combining this test and the one above, but that's up to you.
```
@pytest.mark.parametrize(
"accounts_id, expected_registry",
[
("", DEFAULT_ACCOUNT_ID),
(None, DEFAULT_ACCOUNT_ID),
([], DEFAULT_ACCOUNT_ID),
(DEFAULT_ACCOUNT_ID, DEFAULT_ACCOUNT_ID),
("111100002222", "111100002222"),
(["333366669999"], "333366669999")
]
)
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ferruzzi commented on a diff in pull request #28279: Add Amazon Elastic Container Registry (ECR) Hook
Posted by GitBox <gi...@apache.org>.
ferruzzi commented on code in PR #28279:
URL: https://github.com/apache/airflow/pull/28279#discussion_r1047577892
##########
tests/providers/amazon/aws/hooks/test_ecr.py:
##########
@@ -0,0 +1,103 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+from __future__ import annotations
+
+from unittest import mock
+
+import boto3
+import pytest
+from moto import mock_ecr
+from moto.core import DEFAULT_ACCOUNT_ID
+
+from airflow.providers.amazon.aws.hooks.ecr import EcrHook
+
+
+@pytest.fixture
+def patch_hook(monkeypatch):
+ """Patch hook object by dummy boto3 ECR client."""
+ ecr_client = boto3.client("ecr")
+ monkeypatch.setattr(EcrHook, "conn", ecr_client)
+ yield
+
+
+@mock_ecr
+class TestEcrHook:
+ def test_service_type(self):
+ """Test expected boto3 client type."""
+ assert EcrHook().client_type == "ecr"
+
+ @pytest.mark.parametrize(
+ "accounts_ids",
+ [
+ pytest.param("", id="empty-string"),
+ pytest.param(None, id="none"),
+ pytest.param([], id="empty-list"),
+ ],
+ )
+ def test_get_temporary_credentials_default_account_id(self, patch_hook, accounts_ids):
+ """Test different types of empty account/registry ids."""
+ result = EcrHook().get_temporary_credentials(registry_ids=accounts_ids)
+ assert len(result) == 1
+ assert result[0].username == "AWS"
+ assert result[0].registry.startswith(DEFAULT_ACCOUNT_ID)
+ assert result[0].password == f"{DEFAULT_ACCOUNT_ID}-auth-token"
+
+ @pytest.mark.parametrize(
+ "accounts_id, expected_registry",
+ [
+ pytest.param(DEFAULT_ACCOUNT_ID, DEFAULT_ACCOUNT_ID, id="moto-default-account"),
+ pytest.param("111100002222", "111100002222", id="custom-account-id"),
+ pytest.param(["333366669999"], "333366669999", id="empty-list"),
Review Comment:
If you do keep the id field, then this one is wrong.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] o-nikolas commented on a diff in pull request #28279: Add Amazon Elastic Container Registry (ECR) Hook
Posted by GitBox <gi...@apache.org>.
o-nikolas commented on code in PR #28279:
URL: https://github.com/apache/airflow/pull/28279#discussion_r1051231747
##########
airflow/providers/amazon/aws/hooks/ecr.py:
##########
@@ -0,0 +1,101 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+from __future__ import annotations
+
+import base64
+import logging
+from dataclasses import dataclass
+from datetime import datetime
+
+from airflow.providers.amazon.aws.hooks.base_aws import AwsBaseHook
+from airflow.utils.log.secrets_masker import mask_secret
+
+logger = logging.getLogger(__name__)
+
+
+@dataclass(frozen=True)
+class EcrCredentials:
+ """Helper (frozen dataclass) for storing temporary ECR credentials."""
+
+ username: str
+ password: str
+ proxy_endpoint: str
+ expires_at: datetime
+
+ def __post_init__(self):
+ mask_secret(self.password)
+ logger.debug("Credentials to Amazon ECR %r expires at %s.", self.proxy_endpoint, self.expires_at)
+
+ @property
+ def registry(self) -> str:
+ """Return registry in appropriate `docker login` format."""
+ # https://github.com/docker/docker-py/issues/2256#issuecomment-824940506
+ return self.proxy_endpoint.replace("https://", "")
+
+
+class EcrHook(AwsBaseHook):
+ """
+ Interact with Amazon Elastic Container Registry (ECR)
+
+ Additional arguments (such as ``aws_conn_id``) may be specified and
+ are passed down to the underlying AwsBaseHook.
+
+ .. seealso::
+ :class:`~airflow.providers.amazon.aws.hooks.base_aws.AwsBaseHook`
+ """
+
+ def __init__(self, **kwargs):
+ kwargs.pop("client_type", None)
+ super().__init__(client_type="ecr", **kwargs)
+
+ def get_temporary_credentials(self, registry_ids: list[str] | str | None = None) -> list[EcrCredentials]:
+ """Get temporary credentials for Amazon ECR.
+
+ Return list of :class:`~airflow.providers.amazon.aws.hooks.ecr.EcrCredentials`,
+ obtained credentials valid for 12 hours.
+
+ :param registry_ids: Either AWS Account ID or list of AWS Account IDs that are associated
+ with the registries for which obtain credentials. If you do not specify a registry,
+ the default registry is assumed.
+
+ .. seealso::
+ - `boto3 ECR client get_authorization_token method <https://boto3.amazonaws.com/v1/documentation/\
+ api/latest/reference/services/ecr.html#ECR.Client.get_authorization_token>`_.
+ """
+ registry_ids = registry_ids or None
Review Comment:
Yeah no concern I was just checking to be sure. You could re-arrange the if statements so that the falsey check happens first so that you wouldn't need this assignment, but the way you have it is also perfectly fine :+1:
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ferruzzi commented on a diff in pull request #28279: Add Amazon Elastic Container Registry (ECR) Hook
Posted by GitBox <gi...@apache.org>.
ferruzzi commented on code in PR #28279:
URL: https://github.com/apache/airflow/pull/28279#discussion_r1047517668
##########
airflow/providers/amazon/aws/hooks/ecr.py:
##########
@@ -0,0 +1,101 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+from __future__ import annotations
+
+import base64
+import logging
+from dataclasses import dataclass
+from datetime import datetime
+
+from airflow.providers.amazon.aws.hooks.base_aws import AwsBaseHook
+from airflow.utils.log.secrets_masker import mask_secret
+
+logger = logging.getLogger(__name__)
+
+
+@dataclass(frozen=True)
+class EcrCredentials:
+ """Helper (frozen dataclass) for storing temporary ECR credentials."""
+
+ username: str
+ password: str
+ proxy_endpoint: str
+ expires_at: datetime
+
+ def __post_init__(self):
+ mask_secret(self.password)
+ logger.debug("Credentials to Amazon ECR %r expires at %s.", self.proxy_endpoint, self.expires_at)
+
+ @property
+ def registry(self) -> str:
+ """Return registry in appropriate `docker login` format."""
+ # https://github.com/docker/docker-py/issues/2256#issuecomment-824940506
+ return self.proxy_endpoint.replace("https://", "")
+
+
+class EcrHook(AwsBaseHook):
+ """
+ Interact with Amazon Elastic Container Registry (ECR)
+
+ Additional arguments (such as ``aws_conn_id``) may be specified and
+ are passed down to the underlying AwsBaseHook.
+
+ .. seealso::
+ :class:`~airflow.providers.amazon.aws.hooks.base_aws.AwsBaseHook`
+ """
+
+ def __init__(self, **kwargs):
+ kwargs.pop("client_type", None)
+ super().__init__(client_type="ecr", **kwargs)
+
+ def get_temporary_credentials(self, registry_ids: list[str] | str | None = None) -> list[EcrCredentials]:
+ """Get temporary credentials for Amazon ECR.
+
+ Return list of :class:`~airflow.providers.amazon.aws.hooks.ecr.EcrCredentials`,
+ obtained credentials valid for 12 hours.
+
+ :param registry_ids: Either AWS Account ID or list of AWS Account IDs that are associated
+ with the registries for which obtain credentials. If you do not specify a registry,
+ the default registry is assumed.
+
+ .. seealso::
+ - `boto3 ECR client get_authorization_token method <https://boto3.amazonaws.com/v1/documentation/\
+ api/latest/reference/services/ecr.html#ECR.Client.get_authorization_token>`_.
+ """
+ registry_ids = registry_ids or None
Review Comment:
Is this line doing anything? You're already defaulting to none in the signature.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] Taragolis commented on a diff in pull request #28279: Add Amazon Elastic Container Registry (ECR) Hook
Posted by GitBox <gi...@apache.org>.
Taragolis commented on code in PR #28279:
URL: https://github.com/apache/airflow/pull/28279#discussion_r1047681828
##########
tests/providers/amazon/aws/hooks/test_ecr.py:
##########
@@ -0,0 +1,103 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+from __future__ import annotations
+
+from unittest import mock
+
+import boto3
+import pytest
+from moto import mock_ecr
+from moto.core import DEFAULT_ACCOUNT_ID
+
+from airflow.providers.amazon.aws.hooks.ecr import EcrHook
+
+
+@pytest.fixture
+def patch_hook(monkeypatch):
+ """Patch hook object by dummy boto3 ECR client."""
+ ecr_client = boto3.client("ecr")
+ monkeypatch.setattr(EcrHook, "conn", ecr_client)
+ yield
+
+
+@mock_ecr
+class TestEcrHook:
+ def test_service_type(self):
+ """Test expected boto3 client type."""
+ assert EcrHook().client_type == "ecr"
+
+ @pytest.mark.parametrize(
+ "accounts_ids",
+ [
+ pytest.param("", id="empty-string"),
+ pytest.param(None, id="none"),
+ pytest.param([], id="empty-list"),
+ ],
+ )
Review Comment:
It is more about how it looks in `pytest` output and in PyCharm/IDEA.
More refers how `pytest` work with empty string and objects.
---
```python
@pytest.mark.parametrize("accounts_ids", ["", None, []])
def test_sample(accounts_ids):
pass
```
```console
============================= test session starts ==============================
collecting ... collected 3 items
test_docker.py::test_sample[]
test_docker.py::test_sample[None]
test_docker.py::test_sample[accounts_ids2]
============================== 3 passed in 0.12s ===============================
```
---
```python
@pytest.mark.parametrize(
"accounts_ids",
[
pytest.param("", id="empty-string"),
pytest.param(None, id="none"),
pytest.param([], id="empty-list"),
],
)
def test_sample(accounts_ids):
pass
```
```console
============================= test session starts ==============================
collecting ... collected 3 items
test_docker.py::test_sample[]
test_docker.py::test_sample[None]
test_docker.py::test_sample[accounts_ids2]
============================== 3 passed in 0.12s ===============================
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] Taragolis merged pull request #28279: Add Amazon Elastic Container Registry (ECR) Hook
Posted by GitBox <gi...@apache.org>.
Taragolis merged PR #28279:
URL: https://github.com/apache/airflow/pull/28279
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] Taragolis commented on a diff in pull request #28279: Add Amazon Elastic Container Registry (ECR) Hook
Posted by GitBox <gi...@apache.org>.
Taragolis commented on code in PR #28279:
URL: https://github.com/apache/airflow/pull/28279#discussion_r1047681828
##########
tests/providers/amazon/aws/hooks/test_ecr.py:
##########
@@ -0,0 +1,103 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+from __future__ import annotations
+
+from unittest import mock
+
+import boto3
+import pytest
+from moto import mock_ecr
+from moto.core import DEFAULT_ACCOUNT_ID
+
+from airflow.providers.amazon.aws.hooks.ecr import EcrHook
+
+
+@pytest.fixture
+def patch_hook(monkeypatch):
+ """Patch hook object by dummy boto3 ECR client."""
+ ecr_client = boto3.client("ecr")
+ monkeypatch.setattr(EcrHook, "conn", ecr_client)
+ yield
+
+
+@mock_ecr
+class TestEcrHook:
+ def test_service_type(self):
+ """Test expected boto3 client type."""
+ assert EcrHook().client_type == "ecr"
+
+ @pytest.mark.parametrize(
+ "accounts_ids",
+ [
+ pytest.param("", id="empty-string"),
+ pytest.param(None, id="none"),
+ pytest.param([], id="empty-list"),
+ ],
+ )
Review Comment:
It is more about how it looks in `pytest` output and in PyCharm/IDEA.
More refers how `pytest` work with empty string and objects.
---
```python
@pytest.mark.parametrize("accounts_ids", ["", None, []])
def test_sample(accounts_ids):
pass
```
```console
============================= test session starts ==============================
collecting ... collected 3 items
test_docker.py::test_sample[]
test_docker.py::test_sample[None]
test_docker.py::test_sample[accounts_ids2]
============================== 3 passed in 0.12s ===============================
```
---
```python
@pytest.mark.parametrize(
"accounts_ids",
[
pytest.param("", id="empty-string"),
pytest.param(None, id="none"),
pytest.param([], id="empty-list"),
],
)
def test_sample(accounts_ids):
pass
```
```console
============================= test session starts ==============================
collecting ... collected 3 items
test_docker.py::test_sample[]
test_docker.py::test_sample[None]
test_docker.py::test_sample[accounts_ids2]
============================== 3 passed in 0.12s ===============================
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] Taragolis commented on a diff in pull request #28279: Add Amazon Elastic Container Registry (ECR) Hook
Posted by GitBox <gi...@apache.org>.
Taragolis commented on code in PR #28279:
URL: https://github.com/apache/airflow/pull/28279#discussion_r1047520782
##########
airflow/providers/amazon/aws/hooks/ecr.py:
##########
@@ -0,0 +1,101 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+from __future__ import annotations
+
+import base64
+import logging
+from dataclasses import dataclass
+from datetime import datetime
+
+from airflow.providers.amazon.aws.hooks.base_aws import AwsBaseHook
+from airflow.utils.log.secrets_masker import mask_secret
+
+logger = logging.getLogger(__name__)
+
+
+@dataclass(frozen=True)
+class EcrCredentials:
+ """Helper (frozen dataclass) for storing temporary ECR credentials."""
+
+ username: str
+ password: str
+ proxy_endpoint: str
+ expires_at: datetime
+
+ def __post_init__(self):
+ mask_secret(self.password)
+ logger.debug("Credentials to Amazon ECR %r expires at %s.", self.proxy_endpoint, self.expires_at)
+
+ @property
+ def registry(self) -> str:
+ """Return registry in appropriate `docker login` format."""
+ # https://github.com/docker/docker-py/issues/2256#issuecomment-824940506
+ return self.proxy_endpoint.replace("https://", "")
+
+
+class EcrHook(AwsBaseHook):
+ """
+ Interact with Amazon Elastic Container Registry (ECR)
+
+ Additional arguments (such as ``aws_conn_id``) may be specified and
+ are passed down to the underlying AwsBaseHook.
+
+ .. seealso::
+ :class:`~airflow.providers.amazon.aws.hooks.base_aws.AwsBaseHook`
+ """
+
+ def __init__(self, **kwargs):
+ kwargs.pop("client_type", None)
+ super().__init__(client_type="ecr", **kwargs)
+
+ def get_temporary_credentials(self, registry_ids: list[str] | str | None = None) -> list[EcrCredentials]:
+ """Get temporary credentials for Amazon ECR.
+
+ Return list of :class:`~airflow.providers.amazon.aws.hooks.ecr.EcrCredentials`,
+ obtained credentials valid for 12 hours.
+
+ :param registry_ids: Either AWS Account ID or list of AWS Account IDs that are associated
+ with the registries for which obtain credentials. If you do not specify a registry,
+ the default registry is assumed.
+
+ .. seealso::
+ - `boto3 ECR client get_authorization_token method <https://boto3.amazonaws.com/v1/documentation/\
+ api/latest/reference/services/ecr.html#ECR.Client.get_authorization_token>`_.
+ """
+ registry_ids = registry_ids or None
Review Comment:
Yep, just a guard. This attribute will propagated from connection
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] Taragolis commented on a diff in pull request #28279: Add Amazon Elastic Container Registry (ECR) Hook
Posted by GitBox <gi...@apache.org>.
Taragolis commented on code in PR #28279:
URL: https://github.com/apache/airflow/pull/28279#discussion_r1047691649
##########
tests/providers/amazon/aws/hooks/test_ecr.py:
##########
@@ -0,0 +1,103 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+from __future__ import annotations
+
+from unittest import mock
+
+import boto3
+import pytest
+from moto import mock_ecr
+from moto.core import DEFAULT_ACCOUNT_ID
+
+from airflow.providers.amazon.aws.hooks.ecr import EcrHook
+
+
+@pytest.fixture
+def patch_hook(monkeypatch):
+ """Patch hook object by dummy boto3 ECR client."""
+ ecr_client = boto3.client("ecr")
+ monkeypatch.setattr(EcrHook, "conn", ecr_client)
+ yield
+
+
+@mock_ecr
+class TestEcrHook:
+ def test_service_type(self):
+ """Test expected boto3 client type."""
+ assert EcrHook().client_type == "ecr"
+
+ @pytest.mark.parametrize(
+ "accounts_ids",
+ [
+ pytest.param("", id="empty-string"),
+ pytest.param(None, id="none"),
+ pytest.param([], id="empty-list"),
+ ],
+ )
+ def test_get_temporary_credentials_default_account_id(self, patch_hook, accounts_ids):
+ """Test different types of empty account/registry ids."""
+ result = EcrHook().get_temporary_credentials(registry_ids=accounts_ids)
+ assert len(result) == 1
+ assert result[0].username == "AWS"
+ assert result[0].registry.startswith(DEFAULT_ACCOUNT_ID)
+ assert result[0].password == f"{DEFAULT_ACCOUNT_ID}-auth-token"
+
+ @pytest.mark.parametrize(
+ "accounts_id, expected_registry",
+ [
+ pytest.param(DEFAULT_ACCOUNT_ID, DEFAULT_ACCOUNT_ID, id="moto-default-account"),
+ pytest.param("111100002222", "111100002222", id="custom-account-id"),
+ pytest.param(["333366669999"], "333366669999", id="empty-list"),
+ ],
+ )
+ def test_get_temporary_credentials_single_account_id(self, patch_hook, accounts_id, expected_registry):
Review Comment:
Oh... just split for implicit and explicit account id / regisrty id.
If something changed in `moto`, AWS API (unlikely) or if one day we get rid of this line https://github.com/apache/airflow/pull/28279/files#r1047488028 it might safe a bit of time to realise where actual issue happen
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] Taragolis commented on a diff in pull request #28279: Add Amazon Elastic Container Registry (ECR) Hook
Posted by GitBox <gi...@apache.org>.
Taragolis commented on code in PR #28279:
URL: https://github.com/apache/airflow/pull/28279#discussion_r1047687069
##########
tests/providers/amazon/aws/hooks/test_ecr.py:
##########
@@ -0,0 +1,103 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+from __future__ import annotations
+
+from unittest import mock
+
+import boto3
+import pytest
+from moto import mock_ecr
+from moto.core import DEFAULT_ACCOUNT_ID
+
+from airflow.providers.amazon.aws.hooks.ecr import EcrHook
+
+
+@pytest.fixture
+def patch_hook(monkeypatch):
+ """Patch hook object by dummy boto3 ECR client."""
+ ecr_client = boto3.client("ecr")
+ monkeypatch.setattr(EcrHook, "conn", ecr_client)
+ yield
+
+
+@mock_ecr
+class TestEcrHook:
+ def test_service_type(self):
+ """Test expected boto3 client type."""
+ assert EcrHook().client_type == "ecr"
+
+ @pytest.mark.parametrize(
+ "accounts_ids",
+ [
+ pytest.param("", id="empty-string"),
+ pytest.param(None, id="none"),
+ pytest.param([], id="empty-list"),
+ ],
+ )
Review Comment:
And predictable cli command for run test for single parameter. It's useful during debagging when you change something and just one parameter stop working
```console
❯ pytest path/to.py::TestClassName:test_sample[empty-list]
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] Taragolis commented on a diff in pull request #28279: Add Amazon Elastic Container Registry (ECR) Hook
Posted by GitBox <gi...@apache.org>.
Taragolis commented on code in PR #28279:
URL: https://github.com/apache/airflow/pull/28279#discussion_r1047754974
##########
tests/providers/amazon/aws/hooks/test_ecr.py:
##########
@@ -0,0 +1,103 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+from __future__ import annotations
+
+from unittest import mock
+
+import boto3
+import pytest
+from moto import mock_ecr
+from moto.core import DEFAULT_ACCOUNT_ID
+
+from airflow.providers.amazon.aws.hooks.ecr import EcrHook
+
+
+@pytest.fixture
+def patch_hook(monkeypatch):
+ """Patch hook object by dummy boto3 ECR client."""
+ ecr_client = boto3.client("ecr")
+ monkeypatch.setattr(EcrHook, "conn", ecr_client)
+ yield
+
+
+@mock_ecr
+class TestEcrHook:
+ def test_service_type(self):
+ """Test expected boto3 client type."""
+ assert EcrHook().client_type == "ecr"
+
+ @pytest.mark.parametrize(
+ "accounts_ids",
+ [
+ pytest.param("", id="empty-string"),
+ pytest.param(None, id="none"),
+ pytest.param([], id="empty-list"),
+ ],
+ )
Review Comment:
Yeah... Update it
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ferruzzi commented on a diff in pull request #28279: Add Amazon Elastic Container Registry (ECR) Hook
Posted by GitBox <gi...@apache.org>.
ferruzzi commented on code in PR #28279:
URL: https://github.com/apache/airflow/pull/28279#discussion_r1047503127
##########
airflow/providers/amazon/aws/hooks/ecr.py:
##########
@@ -0,0 +1,101 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+from __future__ import annotations
+
+import base64
+import logging
+from dataclasses import dataclass
+from datetime import datetime
+
+from airflow.providers.amazon.aws.hooks.base_aws import AwsBaseHook
+from airflow.utils.log.secrets_masker import mask_secret
+
+logger = logging.getLogger(__name__)
+
+
+@dataclass(frozen=True)
+class EcrCredentials:
+ """Helper (frozen dataclass) for storing temporary ECR credentials."""
+
+ username: str
+ password: str
+ proxy_endpoint: str
+ expires_at: datetime
+
+ def __post_init__(self):
+ mask_secret(self.password)
+ logger.debug("Credentials to Amazon ECR %r expires at %s.", self.proxy_endpoint, self.expires_at)
+
+ @property
+ def registry(self) -> str:
+ """Return registry in appropriate `docker login` format."""
+ # https://github.com/docker/docker-py/issues/2256#issuecomment-824940506
+ return self.proxy_endpoint.replace("https://", "")
+
+
+class EcrHook(AwsBaseHook):
+ """
+ Interact with Amazon Elastic Container Registry (ECR)
+
+ Additional arguments (such as ``aws_conn_id``) may be specified and
+ are passed down to the underlying AwsBaseHook.
+
+ .. seealso::
+ :class:`~airflow.providers.amazon.aws.hooks.base_aws.AwsBaseHook`
+ """
+
+ def __init__(self, **kwargs):
+ kwargs.pop("client_type", None)
+ super().__init__(client_type="ecr", **kwargs)
Review Comment:
Small nitpick, but I'm always nagging about consistency. I know it's functionally the same thing. Up to you.
```suggestion
kwargs["client_type"] = "ecr"
super().__init__(**kwargs)
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ferruzzi commented on a diff in pull request #28279: Add Amazon Elastic Container Registry (ECR) Hook
Posted by GitBox <gi...@apache.org>.
ferruzzi commented on code in PR #28279:
URL: https://github.com/apache/airflow/pull/28279#discussion_r1047503127
##########
airflow/providers/amazon/aws/hooks/ecr.py:
##########
@@ -0,0 +1,101 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+from __future__ import annotations
+
+import base64
+import logging
+from dataclasses import dataclass
+from datetime import datetime
+
+from airflow.providers.amazon.aws.hooks.base_aws import AwsBaseHook
+from airflow.utils.log.secrets_masker import mask_secret
+
+logger = logging.getLogger(__name__)
+
+
+@dataclass(frozen=True)
+class EcrCredentials:
+ """Helper (frozen dataclass) for storing temporary ECR credentials."""
+
+ username: str
+ password: str
+ proxy_endpoint: str
+ expires_at: datetime
+
+ def __post_init__(self):
+ mask_secret(self.password)
+ logger.debug("Credentials to Amazon ECR %r expires at %s.", self.proxy_endpoint, self.expires_at)
+
+ @property
+ def registry(self) -> str:
+ """Return registry in appropriate `docker login` format."""
+ # https://github.com/docker/docker-py/issues/2256#issuecomment-824940506
+ return self.proxy_endpoint.replace("https://", "")
+
+
+class EcrHook(AwsBaseHook):
+ """
+ Interact with Amazon Elastic Container Registry (ECR)
+
+ Additional arguments (such as ``aws_conn_id``) may be specified and
+ are passed down to the underlying AwsBaseHook.
+
+ .. seealso::
+ :class:`~airflow.providers.amazon.aws.hooks.base_aws.AwsBaseHook`
+ """
+
+ def __init__(self, **kwargs):
+ kwargs.pop("client_type", None)
+ super().__init__(client_type="ecr", **kwargs)
Review Comment:
Small nitpick, but I'm always nagging about consistency. Up to you.
```suggestion
kwargs["client_type"] = "ecr"
super().__init__(**kwargs)
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ferruzzi commented on a diff in pull request #28279: Add Amazon Elastic Container Registry (ECR) Hook
Posted by GitBox <gi...@apache.org>.
ferruzzi commented on code in PR #28279:
URL: https://github.com/apache/airflow/pull/28279#discussion_r1047517668
##########
airflow/providers/amazon/aws/hooks/ecr.py:
##########
@@ -0,0 +1,101 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+from __future__ import annotations
+
+import base64
+import logging
+from dataclasses import dataclass
+from datetime import datetime
+
+from airflow.providers.amazon.aws.hooks.base_aws import AwsBaseHook
+from airflow.utils.log.secrets_masker import mask_secret
+
+logger = logging.getLogger(__name__)
+
+
+@dataclass(frozen=True)
+class EcrCredentials:
+ """Helper (frozen dataclass) for storing temporary ECR credentials."""
+
+ username: str
+ password: str
+ proxy_endpoint: str
+ expires_at: datetime
+
+ def __post_init__(self):
+ mask_secret(self.password)
+ logger.debug("Credentials to Amazon ECR %r expires at %s.", self.proxy_endpoint, self.expires_at)
+
+ @property
+ def registry(self) -> str:
+ """Return registry in appropriate `docker login` format."""
+ # https://github.com/docker/docker-py/issues/2256#issuecomment-824940506
+ return self.proxy_endpoint.replace("https://", "")
+
+
+class EcrHook(AwsBaseHook):
+ """
+ Interact with Amazon Elastic Container Registry (ECR)
+
+ Additional arguments (such as ``aws_conn_id``) may be specified and
+ are passed down to the underlying AwsBaseHook.
+
+ .. seealso::
+ :class:`~airflow.providers.amazon.aws.hooks.base_aws.AwsBaseHook`
+ """
+
+ def __init__(self, **kwargs):
+ kwargs.pop("client_type", None)
+ super().__init__(client_type="ecr", **kwargs)
+
+ def get_temporary_credentials(self, registry_ids: list[str] | str | None = None) -> list[EcrCredentials]:
+ """Get temporary credentials for Amazon ECR.
+
+ Return list of :class:`~airflow.providers.amazon.aws.hooks.ecr.EcrCredentials`,
+ obtained credentials valid for 12 hours.
+
+ :param registry_ids: Either AWS Account ID or list of AWS Account IDs that are associated
+ with the registries for which obtain credentials. If you do not specify a registry,
+ the default registry is assumed.
+
+ .. seealso::
+ - `boto3 ECR client get_authorization_token method <https://boto3.amazonaws.com/v1/documentation/\
+ api/latest/reference/services/ecr.html#ECR.Client.get_authorization_token>`_.
+ """
+ registry_ids = registry_ids or None
Review Comment:
~Is this line doing anything? You're already defaulting to none in the signature.~ Niko replied while I was looking through this, sorry about that
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] Taragolis commented on a diff in pull request #28279: Add Amazon Elastic Container Registry (ECR) Hook
Posted by GitBox <gi...@apache.org>.
Taragolis commented on code in PR #28279:
URL: https://github.com/apache/airflow/pull/28279#discussion_r1050933236
##########
airflow/providers/amazon/aws/hooks/ecr.py:
##########
@@ -0,0 +1,101 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+from __future__ import annotations
+
+import base64
+import logging
+from dataclasses import dataclass
+from datetime import datetime
+
+from airflow.providers.amazon.aws.hooks.base_aws import AwsBaseHook
+from airflow.utils.log.secrets_masker import mask_secret
+
+logger = logging.getLogger(__name__)
+
+
+@dataclass(frozen=True)
+class EcrCredentials:
+ """Helper (frozen dataclass) for storing temporary ECR credentials."""
+
+ username: str
+ password: str
+ proxy_endpoint: str
+ expires_at: datetime
+
+ def __post_init__(self):
+ mask_secret(self.password)
+ logger.debug("Credentials to Amazon ECR %r expires at %s.", self.proxy_endpoint, self.expires_at)
+
+ @property
+ def registry(self) -> str:
+ """Return registry in appropriate `docker login` format."""
+ # https://github.com/docker/docker-py/issues/2256#issuecomment-824940506
+ return self.proxy_endpoint.replace("https://", "")
+
+
+class EcrHook(AwsBaseHook):
+ """
+ Interact with Amazon Elastic Container Registry (ECR)
+
+ Additional arguments (such as ``aws_conn_id``) may be specified and
+ are passed down to the underlying AwsBaseHook.
+
+ .. seealso::
+ :class:`~airflow.providers.amazon.aws.hooks.base_aws.AwsBaseHook`
+ """
+
+ def __init__(self, **kwargs):
+ kwargs.pop("client_type", None)
+ super().__init__(client_type="ecr", **kwargs)
+
+ def get_temporary_credentials(self, registry_ids: list[str] | str | None = None) -> list[EcrCredentials]:
+ """Get temporary credentials for Amazon ECR.
+
+ Return list of :class:`~airflow.providers.amazon.aws.hooks.ecr.EcrCredentials`,
+ obtained credentials valid for 12 hours.
+
+ :param registry_ids: Either AWS Account ID or list of AWS Account IDs that are associated
+ with the registries for which obtain credentials. If you do not specify a registry,
+ the default registry is assumed.
+
+ .. seealso::
+ - `boto3 ECR client get_authorization_token method <https://boto3.amazonaws.com/v1/documentation/\
+ api/latest/reference/services/ecr.html#ECR.Client.get_authorization_token>`_.
+ """
+ registry_ids = registry_ids or None
Review Comment:
@o-nikolas @ferruzzi do we have any concern about this part? Or we could merge this changes?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org