You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "potiuk (via GitHub)" <gi...@apache.org> on 2023/02/12 22:14:06 UTC
[GitHub] [airflow] potiuk opened a new pull request, #29497: Check that cloud sql provider version is valid
potiuk opened a new pull request, #29497:
URL: https://github.com/apache/airflow/pull/29497
Additional chek on cloud sql version should be done to avoid downloading non-existing binary.
<!--
Thank you for contributing! Please make sure that your code changes
are covered with tests. And in case of new features or big changes
remember to adjust the documentation.
Feel free to ping committers for the review!
In case of an existing issue, reference it using one of the following:
closes: #ISSUE
related: #ISSUE
How to write a good git commit message:
http://chris.beams.io/posts/git-commit/
-->
---
**^ Add meaningful description above**
Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst#pull-request-guidelines)** for more information.
In case of fundamental code changes, an Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvement+Proposals)) is needed.
In case of a new dependency, check compliance with the [ASF 3rd Party License Policy](https://www.apache.org/legal/resolved.html#category-x).
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](https://github.com/apache/airflow/tree/main/newsfragments).
--
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] Elonmucks commented on pull request #29497: Check that cloud sql provider version is valid
Posted by "Elonmucks (via GitHub)" <gi...@apache.org>.
Elonmucks commented on PR #29497:
URL: https://github.com/apache/airflow/pull/29497#issuecomment-1531865410
Wow need more cash
--
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 merged pull request #29497: Check that cloud sql provider version is valid
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk merged PR #29497:
URL: https://github.com/apache/airflow/pull/29497
--
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] ashb commented on a diff in pull request #29497: Check that cloud sql provider version is valid
Posted by "ashb (via GitHub)" <gi...@apache.org>.
ashb commented on code in PR #29497:
URL: https://github.com/apache/airflow/pull/29497#discussion_r1104045223
##########
airflow/providers/google/cloud/hooks/cloud_sql.py:
##########
@@ -482,6 +475,24 @@ def _download_sql_proxy_if_needed(self) -> None:
os.chmod(self.sql_proxy_path, 0o744) # Set executable bit
self.sql_proxy_was_downloaded = True
+ def _get_sql_proxy_download_url(self):
+ system = platform.system().lower()
+ processor = os.uname().machine
+ if processor == "x86_64":
+ processor = "amd64"
+ if not self.sql_proxy_version:
+ download_url = CLOUD_SQL_PROXY_DOWNLOAD_URL.format(system, processor)
+ else:
+ if not CLOUD_SQL_PROXY_VERSION_REGEX.match(self.sql_proxy_version):
+ raise Exception(
Review Comment:
Nit:
```suggestion
raise ValueError(
```
##########
tests/providers/google/cloud/hooks/test_cloud_sql.py:
##########
@@ -1185,3 +1200,53 @@ def test_hook_with_correct_parameters_mysql_tcp(self, get_connection):
assert "127.0.0.1" == connection.host
assert 3200 != connection.port
assert "testdb" == connection.schema
+
+
+def get_processor():
+ processor = os.uname().machine
+ if processor == "x86_64":
+ processor = "amd64"
+ return processor
+
+
+class TestCloudSqlProxyRunner:
+ @pytest.mark.parametrize(
+ "version,download_url",
+ [
+ (
+ "v1.23.0",
+ "https://storage.googleapis.com/cloudsql-proxy/v1.23.0/cloud_sql_proxy."
+ f"{platform.system().lower()}.{get_processor()}",
+ ),
+ (
+ "v1.23.0-preview.1",
+ "https://storage.googleapis.com/cloudsql-proxy/v1.23.0-preview.1/cloud_sql_proxy."
+ f"{platform.system().lower()}.{get_processor()}",
+ ),
+ ],
+ )
+ def test_cloud_sql_proxy_runner_version_ok(self, version, download_url):
+ runner = CloudSqlProxyRunner(
+ path_prefix="12345678",
+ instance_specification="project:us-east-1:instance",
+ sql_proxy_version=version,
+ )
+ assert runner._get_sql_proxy_download_url() == download_url
+
+ @pytest.mark.parametrize(
+ "version",
+ [
+ "v1.23.",
+ "v1.23.0..",
+ "v1.23.0\\",
+ "\\",
+ ],
+ )
+ def test_cloud_sql_proxy_runner_version_nok(self, version):
+ runner = CloudSqlProxyRunner(
+ path_prefix="12345678",
+ instance_specification="project:us-east-1:instance",
+ sql_proxy_version=version,
+ )
+ with pytest.raises(Exception, match="The sql_proxy_version should match the regular expression"):
Review Comment:
```suggestion
with pytest.raises(ValueError match="The sql_proxy_version should match the regular expression"):
```
##########
tests/providers/google/cloud/hooks/test_cloud_sql.py:
##########
@@ -1185,3 +1200,53 @@ def test_hook_with_correct_parameters_mysql_tcp(self, get_connection):
assert "127.0.0.1" == connection.host
assert 3200 != connection.port
assert "testdb" == connection.schema
+
+
+def get_processor():
+ processor = os.uname().machine
+ if processor == "x86_64":
+ processor = "amd64"
+ return processor
+
+
+class TestCloudSqlProxyRunner:
+ @pytest.mark.parametrize(
+ "version,download_url",
Review Comment:
```suggestion
["version", "download_url"],
```
--
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 pull request #29497: Check that cloud sql provider version is valid
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #29497:
URL: https://github.com/apache/airflow/pull/29497#issuecomment-1428038685
> Lgtm, just a few nits
Applied. Tests passed. merging.
--
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