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 2020/02/17 10:03:48 UTC
[GitHub] [airflow] nuclearpinguin opened a new pull request #7439:
[AIRFLOW-6204][depends on 6763] Create GoogleSystemTest class
nuclearpinguin opened a new pull request #7439: [AIRFLOW-6204][depends on 6763] Create GoogleSystemTest class
URL: https://github.com/apache/airflow/pull/7439
This PR adds `GoogleSystemTest` class with common helper methods. Also, I refactored all Google system tests to use the new class.
---
Issue link: WILL BE INSERTED BY [boring-cyborg](https://github.com/kaxil/boring-cyborg)
Make sure to mark the boxes below before creating PR: [x]
- [ ] Description above provides context of the change
- [ ] Commit message/PR title starts with `[AIRFLOW-NNNN]`. AIRFLOW-NNNN = JIRA ID<sup>*</sup>
- [ ] Unit tests coverage for changes (not needed for documentation changes)
- [ ] Commits follow "[How to write a good git commit message](http://chris.beams.io/posts/git-commit/)"
- [ ] Relevant documentation is updated including usage instructions.
- [ ] I will engage committers as explained in [Contribution Workflow Example](https://github.com/apache/airflow/blob/master/CONTRIBUTING.rst#contribution-workflow-example).
<sup>*</sup> For document-only changes commit message can start with `[AIRFLOW-XXXX]`.
---
In case of fundamental code change, Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvements+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 [UPDATING.md](https://github.com/apache/airflow/blob/master/UPDATING.md).
Read the [Pull Request Guidelines](https://github.com/apache/airflow/blob/master/CONTRIBUTING.rst#pull-request-guidelines) for more information.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] mik-laj commented on a change in pull request #7439:
[AIRFLOW-6204][depends on 6763] Create GoogleSystemTest class
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #7439: [AIRFLOW-6204][depends on 6763] Create GoogleSystemTest class
URL: https://github.com/apache/airflow/pull/7439#discussion_r380212507
##########
File path: tests/test_utils/gcp_system_helpers.py
##########
@@ -68,3 +77,92 @@ def provide_gcp_context(
return provide_gcp_conn_and_credentials(
key_file_path=key_file_path, scopes=scopes, project_id=project_id
)
+
+
+class GoogleSystemTest(SystemTest):
+ @staticmethod
+ def _project_id():
+ return os.environ.get("GCP_PROJECT_ID")
+
+ @staticmethod
+ def _service_key():
+ return os.environ.get(CREDENTIALS)
+
+ @contextmanager
+ def authentication(self):
+ self.authenticate()
+ yield
+ self.revoke_authentication()
+
+ @classmethod
+ def authenticate(cls):
Review comment:
Can you inline this method in `authentication` context. This will force these methods to be called correctly.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] nuclearpinguin merged pull request #7439: [AIRFLOW-6204]
Create GoogleSystemTest class
Posted by GitBox <gi...@apache.org>.
nuclearpinguin merged pull request #7439: [AIRFLOW-6204] Create GoogleSystemTest class
URL: https://github.com/apache/airflow/pull/7439
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] ashb commented on a change in pull request #7439:
[AIRFLOW-6204][depends on 6763] Create GoogleSystemTest class
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #7439: [AIRFLOW-6204][depends on 6763] Create GoogleSystemTest class
URL: https://github.com/apache/airflow/pull/7439#discussion_r380125997
##########
File path: tests/providers/google/cloud/operators/test_gcs_to_gcs_operator_system.py
##########
@@ -16,33 +16,70 @@
# specific language governing permissions and limitations
# under the License.
"""System tests for Google Cloud Build operators"""
+import pytest
-
-from tests.providers.google.cloud.operators.test_gcs_to_gcs_system_helper import GcsToGcsTestHelper
+from airflow.providers.google.cloud.example_dags.example_gcs_to_gcs import (
+ BUCKET_1_DST, BUCKET_1_SRC, BUCKET_2_DST, BUCKET_2_SRC, BUCKET_3_DST, BUCKET_3_SRC,
+)
from tests.providers.google.cloud.utils.gcp_authenticator import GCP_GCS_KEY
-from tests.test_utils.gcp_system_helpers import CLOUD_DAG_FOLDER, provide_gcp_context, skip_gcp_system
-from tests.test_utils.system_tests_class import SystemTest
+from tests.test_utils.gcp_system_helpers import CLOUD_DAG_FOLDER, GoogleSystemTest, provide_gcp_context
+
+
+@pytest.mark.backend("mysql", "postgres")
+@pytest.mark.system("google.cloud")
+@pytest.mark.credential_file(GCP_GCS_KEY)
+class GcsToGcsExampleDagsSystemTest(GoogleSystemTest):
Review comment:
Can the GoogleSystemTest automatically apply the system mark? Does that work?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] potiuk commented on a change in pull request #7439:
[AIRFLOW-6204][depends on 6763] Create GoogleSystemTest class
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #7439: [AIRFLOW-6204][depends on 6763] Create GoogleSystemTest class
URL: https://github.com/apache/airflow/pull/7439#discussion_r380183421
##########
File path: tests/conftest.py
##########
@@ -159,6 +174,15 @@ def pytest_configure(config):
config.addinivalue_line(
"markers", "runtime(name): mark test to run with named runtime"
)
+ config.addinivalue_line(
+ "markers", "system(name): mark test to run with named system"
+ )
+ config.addinivalue_line(
+ "markers", "long_lasting(name): mark test that run for a long time (many minutes)"
+ )
+ config.addinivalue_line(
+ "markers", "credential_file(name): mark tests that require credential file in CREDENTIALS_DIR"
Review comment:
It makes sense because they also determine whether the test is skipped or not - if the file with credential is missing, then the test is automatically skipped (and reason for skipping explains what credential is missing). I think it's generic enough to be used in a number of cases (and we can include other markers for that - credential_variable for example for env variables missing). The point here is to skip the tests when the credentials are missing.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] potiuk commented on a change in pull request #7439:
[AIRFLOW-6204][depends on 6763] Create GoogleSystemTest class
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #7439: [AIRFLOW-6204][depends on 6763] Create GoogleSystemTest class
URL: https://github.com/apache/airflow/pull/7439#discussion_r380888600
##########
File path: tests/conftest.py
##########
@@ -87,6 +88,26 @@ def pytest_addoption(parser):
metavar="RUNTIME",
help="only run tests matching the runtime: [kubernetes].",
)
+ group.addoption(
+ "--systems",
+ action="store",
+ metavar="SYSTEMS",
+ help="only run tests matching the systems specified [google.cloud, google.marketing_platform]",
+ )
+ group.addoption(
+ "--include-long-lasting",
Review comment:
Updated in my change.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] codecov-io commented on issue #7439: [AIRFLOW-6204]
Create GoogleSystemTest class
Posted by GitBox <gi...@apache.org>.
codecov-io commented on issue #7439: [AIRFLOW-6204] Create GoogleSystemTest class
URL: https://github.com/apache/airflow/pull/7439#issuecomment-589956557
# [Codecov](https://codecov.io/gh/apache/airflow/pull/7439?src=pr&el=h1) Report
> Merging [#7439](https://codecov.io/gh/apache/airflow/pull/7439?src=pr&el=desc) into [master](https://codecov.io/gh/apache/airflow/commit/61a8bb65818521ccbb846e647103535b3e36b26d?src=pr&el=desc) will **decrease** coverage by `0.86%`.
> The diff coverage is `n/a`.
[](https://codecov.io/gh/apache/airflow/pull/7439?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #7439 +/- ##
==========================================
- Coverage 86.82% 85.96% -0.87%
==========================================
Files 891 891
Lines 42095 42095
==========================================
- Hits 36549 36185 -364
- Misses 5546 5910 +364
```
| [Impacted Files](https://codecov.io/gh/apache/airflow/pull/7439?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [...flow/providers/apache/cassandra/hooks/cassandra.py](https://codecov.io/gh/apache/airflow/pull/7439/diff?src=pr&el=tree#diff-YWlyZmxvdy9wcm92aWRlcnMvYXBhY2hlL2Nhc3NhbmRyYS9ob29rcy9jYXNzYW5kcmEucHk=) | `21.51% <0%> (-72.16%)` | :arrow_down: |
| [airflow/kubernetes/volume\_mount.py](https://codecov.io/gh/apache/airflow/pull/7439/diff?src=pr&el=tree#diff-YWlyZmxvdy9rdWJlcm5ldGVzL3ZvbHVtZV9tb3VudC5weQ==) | `44.44% <0%> (-55.56%)` | :arrow_down: |
| [airflow/providers/postgres/operators/postgres.py](https://codecov.io/gh/apache/airflow/pull/7439/diff?src=pr&el=tree#diff-YWlyZmxvdy9wcm92aWRlcnMvcG9zdGdyZXMvb3BlcmF0b3JzL3Bvc3RncmVzLnB5) | `50% <0%> (-50%)` | :arrow_down: |
| [airflow/providers/redis/operators/redis\_publish.py](https://codecov.io/gh/apache/airflow/pull/7439/diff?src=pr&el=tree#diff-YWlyZmxvdy9wcm92aWRlcnMvcmVkaXMvb3BlcmF0b3JzL3JlZGlzX3B1Ymxpc2gucHk=) | `50% <0%> (-50%)` | :arrow_down: |
| [airflow/kubernetes/volume.py](https://codecov.io/gh/apache/airflow/pull/7439/diff?src=pr&el=tree#diff-YWlyZmxvdy9rdWJlcm5ldGVzL3ZvbHVtZS5weQ==) | `52.94% <0%> (-47.06%)` | :arrow_down: |
| [airflow/providers/mongo/sensors/mongo.py](https://codecov.io/gh/apache/airflow/pull/7439/diff?src=pr&el=tree#diff-YWlyZmxvdy9wcm92aWRlcnMvbW9uZ28vc2Vuc29ycy9tb25nby5weQ==) | `53.33% <0%> (-46.67%)` | :arrow_down: |
| [airflow/kubernetes/pod\_launcher.py](https://codecov.io/gh/apache/airflow/pull/7439/diff?src=pr&el=tree#diff-YWlyZmxvdy9rdWJlcm5ldGVzL3BvZF9sYXVuY2hlci5weQ==) | `47.18% <0%> (-45.08%)` | :arrow_down: |
| [airflow/executors/sequential\_executor.py](https://codecov.io/gh/apache/airflow/pull/7439/diff?src=pr&el=tree#diff-YWlyZmxvdy9leGVjdXRvcnMvc2VxdWVudGlhbF9leGVjdXRvci5weQ==) | `56% <0%> (-44%)` | :arrow_down: |
| [airflow/providers/redis/sensors/redis\_key.py](https://codecov.io/gh/apache/airflow/pull/7439/diff?src=pr&el=tree#diff-YWlyZmxvdy9wcm92aWRlcnMvcmVkaXMvc2Vuc29ycy9yZWRpc19rZXkucHk=) | `61.53% <0%> (-38.47%)` | :arrow_down: |
| [airflow/executors/celery\_executor.py](https://codecov.io/gh/apache/airflow/pull/7439/diff?src=pr&el=tree#diff-YWlyZmxvdy9leGVjdXRvcnMvY2VsZXJ5X2V4ZWN1dG9yLnB5) | `50.67% <0%> (-37.84%)` | :arrow_down: |
| ... and [11 more](https://codecov.io/gh/apache/airflow/pull/7439/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/airflow/pull/7439?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/airflow/pull/7439?src=pr&el=footer). Last update [61a8bb6...11b71bb](https://codecov.io/gh/apache/airflow/pull/7439?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] mik-laj commented on a change in pull request #7439:
[AIRFLOW-6204][depends on 6763] Create GoogleSystemTest class
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #7439: [AIRFLOW-6204][depends on 6763] Create GoogleSystemTest class
URL: https://github.com/apache/airflow/pull/7439#discussion_r380213580
##########
File path: tests/test_utils/gcp_system_helpers.py
##########
@@ -68,3 +77,92 @@ def provide_gcp_context(
return provide_gcp_conn_and_credentials(
key_file_path=key_file_path, scopes=scopes, project_id=project_id
)
+
+
+class GoogleSystemTest(SystemTest):
+ @staticmethod
+ def _project_id():
+ return os.environ.get("GCP_PROJECT_ID")
+
+ @staticmethod
+ def _service_key():
+ return os.environ.get(CREDENTIALS)
+
+ @contextmanager
+ def authentication(self):
+ self.authenticate()
+ yield
+ self.revoke_authentication()
+
+ @classmethod
+ def authenticate(cls):
+ """
+ Authenticate with service account specified via key name.
+ Required only when we use gcloud / gsutil.
+ """
+ cls.executor.execute_cmd(
+ [
+ "gcloud",
+ "auth",
+ "activate-service-account",
+ f"--key-file={cls._service_key()}",
+ f"--project={cls._project_id()}",
+ ]
+ )
+
+ @classmethod
+ def revoke_authentication(cls):
+ """
+ Change default authentication to none - which is not existing one.
+ """
+ cls.executor.execute_cmd(
+ [
+ "gcloud",
+ "config",
+ "set",
+ "account",
+ "none",
+ f"--project={cls._project_id()}",
+ ]
+ )
+
+ @classmethod
+ def execute_with_ctx(cls, cmd: List[str], key: str = GCP_GCS_KEY):
+ """
+ Executes command with context created by provide_gcp_context and activated
+ service key.
+ """
+ with provide_gcp_context(key):
+ cls.authenticate()
+ env = os.environ.copy()
+ cls.executor.execute_cmd(cmd=cmd, env=env)
+ cls.revoke_authentication()
+
+ @classmethod
+ def create_gcs_bucket(cls, name: str, location: Optional[str] = None) -> None:
+ cmd = ["gsutil", "mb"]
+ if location:
+ cmd += ["-c", "regional", "-l", location]
+ cmd += [f"gs://{name}"]
+ cls.execute_with_ctx(cmd, key=GCP_GCS_KEY)
+
+ @classmethod
+ def delete_gcs_bucket(cls, name: str):
+ cmd = ["gsutil", "-m", "rm", "-r", f"gs://{name}"]
+ cls.execute_with_ctx(cmd, key=GCP_GCS_KEY)
+
+ @classmethod
+ def upload_to_gcs(cls, source_uri: str, target_uri: str):
+ cls.execute_with_ctx(
+ ["gsutil", "cp", f"{target_uri}", f"{source_uri}"], key=GCP_GCS_KEY
+ )
+
+ @classmethod
+ def upload_content_to_gcs(cls, lines: List[str], bucket_uri: str, filename: str):
Review comment:
```suggestion
def upload_content_to_gcs(cls, content: str, bucket_uri: str, filename: str):
```
WDYT? I think it will be easier to use. Performance is not critical in this case. A simple types is also better.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] nuclearpinguin commented on a change in pull request
#7439: [AIRFLOW-6204][depends on 6763] Create GoogleSystemTest class
Posted by GitBox <gi...@apache.org>.
nuclearpinguin commented on a change in pull request #7439: [AIRFLOW-6204][depends on 6763] Create GoogleSystemTest class
URL: https://github.com/apache/airflow/pull/7439#discussion_r380624263
##########
File path: tests/providers/google/cloud/operators/test_gcs_to_gcs_operator_system.py
##########
@@ -16,33 +16,70 @@
# specific language governing permissions and limitations
# under the License.
"""System tests for Google Cloud Build operators"""
+import pytest
-
-from tests.providers.google.cloud.operators.test_gcs_to_gcs_system_helper import GcsToGcsTestHelper
+from airflow.providers.google.cloud.example_dags.example_gcs_to_gcs import (
+ BUCKET_1_DST, BUCKET_1_SRC, BUCKET_2_DST, BUCKET_2_SRC, BUCKET_3_DST, BUCKET_3_SRC,
+)
from tests.providers.google.cloud.utils.gcp_authenticator import GCP_GCS_KEY
-from tests.test_utils.gcp_system_helpers import CLOUD_DAG_FOLDER, provide_gcp_context, skip_gcp_system
-from tests.test_utils.system_tests_class import SystemTest
+from tests.test_utils.gcp_system_helpers import CLOUD_DAG_FOLDER, GoogleSystemTest, provide_gcp_context
+
+
+@pytest.mark.backend("mysql", "postgres")
+@pytest.mark.system("google.cloud")
+@pytest.mark.credential_file(GCP_GCS_KEY)
+class GcsToGcsExampleDagsSystemTest(GoogleSystemTest):
Review comment:
If `GoogleSystemTest` is marked with system test then all inheriting classes will be marked as well.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] potiuk commented on a change in pull request #7439:
[AIRFLOW-6204][depends on 6763] Create GoogleSystemTest class
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #7439: [AIRFLOW-6204][depends on 6763] Create GoogleSystemTest class
URL: https://github.com/apache/airflow/pull/7439#discussion_r380182209
##########
File path: tests/conftest.py
##########
@@ -87,6 +88,26 @@ def pytest_addoption(parser):
metavar="RUNTIME",
help="only run tests matching the runtime: [kubernetes].",
)
+ group.addoption(
+ "--systems",
+ action="store",
+ metavar="SYSTEMS",
+ help="only run tests matching the systems specified [google.cloud, google.marketing_platform]",
+ )
+ group.addoption(
+ "--include-long-lasting",
Review comment:
Good point. I will update it in my change it comes from #7388
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] ashb commented on a change in pull request #7439:
[AIRFLOW-6204][depends on 6763] Create GoogleSystemTest class
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #7439: [AIRFLOW-6204][depends on 6763] Create GoogleSystemTest class
URL: https://github.com/apache/airflow/pull/7439#discussion_r380144171
##########
File path: tests/conftest.py
##########
@@ -159,6 +174,15 @@ def pytest_configure(config):
config.addinivalue_line(
"markers", "runtime(name): mark test to run with named runtime"
)
+ config.addinivalue_line(
+ "markers", "system(name): mark test to run with named system"
+ )
+ config.addinivalue_line(
+ "markers", "long_lasting(name): mark test that run for a long time (many minutes)"
+ )
+ config.addinivalue_line(
+ "markers", "credential_file(name): mark tests that require credential file in CREDENTIALS_DIR"
Review comment:
Does it make sense for credential_file to be a "marker" in the pytest-sense, This isn't something I'd imagine we'd select by on the command line (but system etc do make sense as markers).
Are these perhaps instead just a property that should be set on the SystemTest sub-class?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] nuclearpinguin commented on a change in pull request
#7439: [AIRFLOW-6204][depends on 6763] Create GoogleSystemTest class
Posted by GitBox <gi...@apache.org>.
nuclearpinguin commented on a change in pull request #7439: [AIRFLOW-6204][depends on 6763] Create GoogleSystemTest class
URL: https://github.com/apache/airflow/pull/7439#discussion_r380525264
##########
File path: tests/test_utils/system_tests_class.py
##########
@@ -106,6 +107,11 @@ def _restore_dags_from_temporary_directory(
class SystemTest(TestCase, LoggingMixin):
+ executor = LoggingCommandExecutor()
Review comment:
I would prefer to do this as next PR
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] nuclearpinguin commented on a change in pull request
#7439: [AIRFLOW-6204][depends on 6763] Create GoogleSystemTest class
Posted by GitBox <gi...@apache.org>.
nuclearpinguin commented on a change in pull request #7439: [AIRFLOW-6204][depends on 6763] Create GoogleSystemTest class
URL: https://github.com/apache/airflow/pull/7439#discussion_r380601084
##########
File path: tests/test_utils/gcp_system_helpers.py
##########
@@ -68,3 +77,92 @@ def provide_gcp_context(
return provide_gcp_conn_and_credentials(
key_file_path=key_file_path, scopes=scopes, project_id=project_id
)
+
+
+class GoogleSystemTest(SystemTest):
+ @staticmethod
+ def _project_id():
+ return os.environ.get("GCP_PROJECT_ID")
+
+ @staticmethod
Review comment:
Property cannot be accessed by classmethod. I am using classmethods because it will help in future to create pytest fixtures
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] mik-laj commented on a change in pull request #7439:
[AIRFLOW-6204][depends on 6763] Create GoogleSystemTest class
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #7439: [AIRFLOW-6204][depends on 6763] Create GoogleSystemTest class
URL: https://github.com/apache/airflow/pull/7439#discussion_r380207830
##########
File path: tests/test_utils/system_tests_class.py
##########
@@ -232,3 +238,20 @@ def run_dag(self, dag_id: str, dag_folder: str = DEFAULT_DAG_FOLDER) -> None:
except Exception:
self._print_all_log_files()
raise
+
+ @staticmethod
+ def create_temp_file(filename, dir_path="/tmp"):
Review comment:
```suggestion
def create_dummy_file(filename, dir_path="/tmp"):
```
Temporary files are files that are used for a short time. However, they can contain valuable content.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] mik-laj commented on a change in pull request #7439:
[AIRFLOW-6204][depends on 6763] Create GoogleSystemTest class
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #7439: [AIRFLOW-6204][depends on 6763] Create GoogleSystemTest class
URL: https://github.com/apache/airflow/pull/7439#discussion_r380210799
##########
File path: tests/test_utils/gcp_system_helpers.py
##########
@@ -68,3 +77,92 @@ def provide_gcp_context(
return provide_gcp_conn_and_credentials(
key_file_path=key_file_path, scopes=scopes, project_id=project_id
)
+
+
+class GoogleSystemTest(SystemTest):
+ @staticmethod
+ def _project_id():
+ return os.environ.get("GCP_PROJECT_ID")
+
+ @staticmethod
+ def _service_key():
+ return os.environ.get(CREDENTIALS)
+
+ @contextmanager
+ def authentication(self):
+ self.authenticate()
+ yield
Review comment:
```suggestion
try:
yield
finally:
self.revoke_authentication()
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] mik-laj commented on a change in pull request #7439:
[AIRFLOW-6204][depends on 6763] Create GoogleSystemTest class
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #7439: [AIRFLOW-6204][depends on 6763] Create GoogleSystemTest class
URL: https://github.com/apache/airflow/pull/7439#discussion_r380211823
##########
File path: tests/test_utils/gcp_system_helpers.py
##########
@@ -68,3 +77,92 @@ def provide_gcp_context(
return provide_gcp_conn_and_credentials(
key_file_path=key_file_path, scopes=scopes, project_id=project_id
)
+
+
+class GoogleSystemTest(SystemTest):
+ @staticmethod
+ def _project_id():
+ return os.environ.get("GCP_PROJECT_ID")
+
+ @staticmethod
Review comment:
```suggestion
@property
```
It's a simple getter. so it should be 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] ashb commented on a change in pull request #7439:
[AIRFLOW-6204][depends on 6763] Create GoogleSystemTest class
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #7439: [AIRFLOW-6204][depends on 6763] Create GoogleSystemTest class
URL: https://github.com/apache/airflow/pull/7439#discussion_r380138484
##########
File path: tests/conftest.py
##########
@@ -87,6 +88,26 @@ def pytest_addoption(parser):
metavar="RUNTIME",
help="only run tests matching the runtime: [kubernetes].",
)
+ group.addoption(
+ "--systems",
+ action="store",
+ metavar="SYSTEMS",
+ help="only run tests matching the systems specified [google.cloud, google.marketing_platform]",
+ )
+ group.addoption(
+ "--include-long-lasting",
Review comment:
```suggestion
"--include-long-running",
```
Long lasting means something subtly different - since we are talking about the duration of the tests, it is the run (time) that we are describing.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] mik-laj commented on a change in pull request #7439:
[AIRFLOW-6204][depends on 6763] Create GoogleSystemTest class
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #7439: [AIRFLOW-6204][depends on 6763] Create GoogleSystemTest class
URL: https://github.com/apache/airflow/pull/7439#discussion_r380209148
##########
File path: tests/test_utils/system_tests_class.py
##########
@@ -106,6 +107,11 @@ def _restore_dags_from_temporary_directory(
class SystemTest(TestCase, LoggingMixin):
+ executor = LoggingCommandExecutor()
Review comment:
LoggingCommandExecutor contains only static methods. It may be worth changing to raw functions so that you do not have to create an instance of this class.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] mik-laj commented on a change in pull request #7439:
[AIRFLOW-6204][depends on 6763] Create GoogleSystemTest class
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #7439: [AIRFLOW-6204][depends on 6763] Create GoogleSystemTest class
URL: https://github.com/apache/airflow/pull/7439#discussion_r380211437
##########
File path: tests/test_utils/gcp_system_helpers.py
##########
@@ -68,3 +77,92 @@ def provide_gcp_context(
return provide_gcp_conn_and_credentials(
key_file_path=key_file_path, scopes=scopes, project_id=project_id
)
+
+
+class GoogleSystemTest(SystemTest):
+ @staticmethod
+ def _project_id():
+ return os.environ.get("GCP_PROJECT_ID")
+
+ @staticmethod
+ def _service_key():
+ return os.environ.get(CREDENTIALS)
+
+ @contextmanager
+ def authentication(self):
+ self.authenticate()
+ yield
+ self.revoke_authentication()
+
+ @classmethod
+ def authenticate(cls):
+ """
+ Authenticate with service account specified via key name.
+ Required only when we use gcloud / gsutil.
+ """
+ cls.executor.execute_cmd(
+ [
+ "gcloud",
+ "auth",
+ "activate-service-account",
+ f"--key-file={cls._service_key()}",
+ f"--project={cls._project_id()}",
+ ]
+ )
+
+ @classmethod
+ def revoke_authentication(cls):
+ """
+ Change default authentication to none - which is not existing one.
+ """
+ cls.executor.execute_cmd(
+ [
+ "gcloud",
+ "config",
+ "set",
+ "account",
+ "none",
+ f"--project={cls._project_id()}",
+ ]
+ )
+
+ @classmethod
+ def execute_with_ctx(cls, cmd: List[str], key: str = GCP_GCS_KEY):
+ """
+ Executes command with context created by provide_gcp_context and activated
+ service key.
+ """
+ with provide_gcp_context(key):
Review comment:
```suggestion
with provide_gcp_context(key), cls.authentication():
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] potiuk commented on a change in pull request #7439:
[AIRFLOW-6204][depends on 6763] Create GoogleSystemTest class
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #7439: [AIRFLOW-6204][depends on 6763] Create GoogleSystemTest class
URL: https://github.com/apache/airflow/pull/7439#discussion_r380193365
##########
File path: tests/providers/google/cloud/operators/test_gcs_to_gcs_operator_system.py
##########
@@ -16,33 +16,70 @@
# specific language governing permissions and limitations
# under the License.
"""System tests for Google Cloud Build operators"""
+import pytest
-
-from tests.providers.google.cloud.operators.test_gcs_to_gcs_system_helper import GcsToGcsTestHelper
+from airflow.providers.google.cloud.example_dags.example_gcs_to_gcs import (
+ BUCKET_1_DST, BUCKET_1_SRC, BUCKET_2_DST, BUCKET_2_SRC, BUCKET_3_DST, BUCKET_3_SRC,
+)
from tests.providers.google.cloud.utils.gcp_authenticator import GCP_GCS_KEY
-from tests.test_utils.gcp_system_helpers import CLOUD_DAG_FOLDER, provide_gcp_context, skip_gcp_system
-from tests.test_utils.system_tests_class import SystemTest
+from tests.test_utils.gcp_system_helpers import CLOUD_DAG_FOLDER, GoogleSystemTest, provide_gcp_context
+
+
+@pytest.mark.backend("mysql", "postgres")
+@pytest.mark.system("google.cloud")
+@pytest.mark.credential_file(GCP_GCS_KEY)
+class GcsToGcsExampleDagsSystemTest(GoogleSystemTest):
Review comment:
I think it would not work, but something that is worth checking @nuclearpinguin :)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] mik-laj commented on a change in pull request #7439:
[AIRFLOW-6204][depends on 6763] Create GoogleSystemTest class
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #7439: [AIRFLOW-6204][depends on 6763] Create GoogleSystemTest class
URL: https://github.com/apache/airflow/pull/7439#discussion_r380211911
##########
File path: tests/test_utils/gcp_system_helpers.py
##########
@@ -68,3 +77,92 @@ def provide_gcp_context(
return provide_gcp_conn_and_credentials(
key_file_path=key_file_path, scopes=scopes, project_id=project_id
)
+
+
+class GoogleSystemTest(SystemTest):
+ @staticmethod
Review comment:
```suggestion
@property
```
It's a simple getter. so it should be 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] potiuk commented on a change in pull request #7439:
[AIRFLOW-6204][depends on 6763] Create GoogleSystemTest class
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #7439: [AIRFLOW-6204][depends on 6763] Create GoogleSystemTest class
URL: https://github.com/apache/airflow/pull/7439#discussion_r380193365
##########
File path: tests/providers/google/cloud/operators/test_gcs_to_gcs_operator_system.py
##########
@@ -16,33 +16,70 @@
# specific language governing permissions and limitations
# under the License.
"""System tests for Google Cloud Build operators"""
+import pytest
-
-from tests.providers.google.cloud.operators.test_gcs_to_gcs_system_helper import GcsToGcsTestHelper
+from airflow.providers.google.cloud.example_dags.example_gcs_to_gcs import (
+ BUCKET_1_DST, BUCKET_1_SRC, BUCKET_2_DST, BUCKET_2_SRC, BUCKET_3_DST, BUCKET_3_SRC,
+)
from tests.providers.google.cloud.utils.gcp_authenticator import GCP_GCS_KEY
-from tests.test_utils.gcp_system_helpers import CLOUD_DAG_FOLDER, provide_gcp_context, skip_gcp_system
-from tests.test_utils.system_tests_class import SystemTest
+from tests.test_utils.gcp_system_helpers import CLOUD_DAG_FOLDER, GoogleSystemTest, provide_gcp_context
+
+
+@pytest.mark.backend("mysql", "postgres")
+@pytest.mark.system("google.cloud")
+@pytest.mark.credential_file(GCP_GCS_KEY)
+class GcsToGcsExampleDagsSystemTest(GoogleSystemTest):
Review comment:
I think it would not work (I think markers are not , but something that is worth checking @nuclearpinguin :)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services