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/06/24 19:19:47 UTC
[GitHub] [airflow] o-nikolas opened a new pull request, #24643: Convert Step Functions Example DAG to System Test (AIP-47)
o-nikolas opened a new pull request, #24643:
URL: https://github.com/apache/airflow/pull/24643
This PR moves another AWS example dag to a system test implementation. This example also includes a new test context builder class which produces a TaskFlow task that computes, fetches and stores test config at runtime instead of module load time, since doing novel work (including network requests) at module load time causes a couple specific issues for these tests and is generally bad practice for dags to be doing such work on dag parse/load time.
Related: #22438
<!--
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 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 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 a newsfragement file, named `{pr_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] potiuk merged pull request #24643: Convert Step Functions Example DAG to System Test (AIP-47)
Posted by GitBox <gi...@apache.org>.
potiuk merged PR #24643:
URL: https://github.com/apache/airflow/pull/24643
--
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 #24643: Convert Step Functions Example DAG to System Test (AIP-47)
Posted by GitBox <gi...@apache.org>.
o-nikolas commented on code in PR #24643:
URL: https://github.com/apache/airflow/pull/24643#discussion_r913321709
##########
tests/system/providers/amazon/aws/utils/__init__.py:
##########
@@ -102,7 +105,54 @@ def _fetch_from_ssm(key: str) -> str:
return value
-def fetch_variable(key: str, default_value: Optional[str] = None) -> str:
+class SystemTestContextBuilder:
+ """This builder class ultimately constructs a TaskFlow task which is run at
+ runtime (task execution time). This task generates and stores the test ENV_ID as well
+ as any external resources requested (e.g.g IAM Roles, VPC, etc)"""
+
+ def __init__(self):
+ self.variables = []
+ self.variable_defaults = {}
+ self.test_name = _get_test_name()
+ self.env_id = set_env_id()
+
+ def add_variable(self, variable_name: str, **kwargs):
+ """Register a variable to fetch from environment or cloud parameter store"""
+ self.variables.append(variable_name)
+ # default_value is accepted via kwargs so that it is completely optional and no
+ # default value needs to be provided in the method stub (otherwise we wouldn't
+ # be able to tell the difference between our default value and one provided by
+ # the caller)
+ if 'default_value' in kwargs:
+ self.variable_defaults[variable_name] = kwargs['default_value']
+
+ return self # Builder recipe; returning self allows chaining
+
+ def build(self):
+ """Build and return a TaskFlow task which will create an env_id and
+ fetch requested variables. Storing everything in xcom for downstream
+ tasks to use."""
+
+ @task
+ def variable_fetcher(**kwargs):
+ ti = kwargs['ti']
+ for variable in self.variables:
+ if variable in self.variable_defaults:
+ value = fetch_variable(
+ variable, self.variable_defaults[variable], test_name=self.test_name
+ )
+ else:
+ value = fetch_variable(variable, test_name=self.test_name)
Review Comment:
This code was written in such a way that any default could be provided, even `None` (since the user may want literally `None` to be the default value), but you're right that the `fetch_variable` code wasn't written with that philosophy so we can go with your suggestion for now until someone requires otherwise.
--
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] bhirsz commented on pull request #24643: Convert Step Functions Example DAG to System Test (AIP-47)
Posted by GitBox <gi...@apache.org>.
bhirsz commented on PR #24643:
URL: https://github.com/apache/airflow/pull/24643#issuecomment-1169640596
Looks nice! So the TaskFlow is used to manage system test configuration. What will happen if TaskFlow itself fails, will it be clear to users that it failed in _setup_ phase rather than in system tests (of any other service)?
--
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 pull request #24643: Convert Step Functions Example DAG to System Test (AIP-47)
Posted by GitBox <gi...@apache.org>.
o-nikolas commented on PR #24643:
URL: https://github.com/apache/airflow/pull/24643#issuecomment-1174506896
> Looks nice! So the TaskFlow is used to manage system test configuration. What will happen if TaskFlow itself fails, will it be clear to users that it failed in _setup_ phase rather than in system tests (of any other service)?
I'm not sure which TaskFlow failures you're imagining here specifically, but one should get an exception like you would from any other catastrophic failure and they should be able to tell from that where the issue occurred. I'm happy to place some more error handling somewhere specific if you have any suggestions!
--
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 pull request #24643: Convert Step Functions Example DAG to System Test (AIP-47)
Posted by GitBox <gi...@apache.org>.
o-nikolas commented on PR #24643:
URL: https://github.com/apache/airflow/pull/24643#issuecomment-1169254637
@potiuk and @bhirsz do you have some spare cycles to look at this one?
--
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] bhirsz commented on a diff in pull request #24643: Convert Step Functions Example DAG to System Test (AIP-47)
Posted by GitBox <gi...@apache.org>.
bhirsz commented on code in PR #24643:
URL: https://github.com/apache/airflow/pull/24643#discussion_r909298070
##########
tests/system/providers/amazon/aws/utils/__init__.py:
##########
@@ -102,7 +105,54 @@ def _fetch_from_ssm(key: str) -> str:
return value
-def fetch_variable(key: str, default_value: Optional[str] = None) -> str:
+class SystemTestContextBuilder:
+ """This builder class ultimately constructs a TaskFlow task which is run at
+ runtime (task execution time). This task generates and stores the test ENV_ID as well
+ as any external resources requested (e.g.g IAM Roles, VPC, etc)"""
+
+ def __init__(self):
+ self.variables = []
+ self.variable_defaults = {}
+ self.test_name = _get_test_name()
+ self.env_id = set_env_id()
+
+ def add_variable(self, variable_name: str, **kwargs):
+ """Register a variable to fetch from environment or cloud parameter store"""
+ self.variables.append(variable_name)
+ # default_value is accepted via kwargs so that it is completely optional and no
+ # default value needs to be provided in the method stub (otherwise we wouldn't
+ # be able to tell the difference between our default value and one provided by
+ # the caller)
+ if 'default_value' in kwargs:
+ self.variable_defaults[variable_name] = kwargs['default_value']
+
+ return self # Builder recipe; returning self allows chaining
+
+ def build(self):
+ """Build and return a TaskFlow task which will create an env_id and
+ fetch requested variables. Storing everything in xcom for downstream
+ tasks to use."""
+
+ @task
+ def variable_fetcher(**kwargs):
+ ti = kwargs['ti']
+ for variable in self.variables:
+ if variable in self.variable_defaults:
+ value = fetch_variable(
+ variable, self.variable_defaults[variable], test_name=self.test_name
+ )
+ else:
+ value = fetch_variable(variable, test_name=self.test_name)
Review Comment:
since fetch_variable defaults default_value to None, we can just do this:
```suggestion
default_value = self.variable_defaults.get(variable, None)
value = fetch_variable(variable, default_value, test_name=self.test_name)
```
--
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 pull request #24643: Convert Step Functions Example DAG to System Test (AIP-47)
Posted by GitBox <gi...@apache.org>.
o-nikolas commented on PR #24643:
URL: https://github.com/apache/airflow/pull/24643#issuecomment-1176673421
@bhirsz I made the updates you suggested, ready for another review :) (CC @potiuk)
--
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