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/04/29 12:28:39 UTC
[GitHub] [airflow] tirkarthi opened a new pull request, #23359: Validate DAG owner to be a string.
tirkarthi opened a new pull request, #23359:
URL: https://github.com/apache/airflow/pull/23359
non-string values raise `AttributeError` as `task.owner.lower` is called with `task.owner` not being a string and the error is not passed as import error failing silently. Raise explicit error will be helpful to the user.
closes: #23343
related: #23343
--
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] mik-laj commented on a diff in pull request #23359: Validate DAG owner to be a string.
Posted by GitBox <gi...@apache.org>.
mik-laj commented on code in PR #23359:
URL: https://github.com/apache/airflow/pull/23359#discussion_r861997869
##########
tests/cluster_policies/__init__.py:
##########
@@ -27,6 +27,9 @@
# [START example_cluster_policy_rule]
def task_must_have_owners(task: BaseOperator):
+ if task.owner and not isinstance(task.owner, str):
+ raise AirflowClusterPolicyViolation(f'''owner should be a string. Current value: {task.owner}''')
Review Comment:
```suggestion
raise AirflowClusterPolicyViolation(f'''owner should be a string. Current value: {task.owner!r}''')
```
--
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] tirkarthi commented on a diff in pull request #23359: Validate DAG owner to be a string.
Posted by GitBox <gi...@apache.org>.
tirkarthi commented on code in PR #23359:
URL: https://github.com/apache/airflow/pull/23359#discussion_r862021988
##########
tests/cluster_policies/__init__.py:
##########
@@ -27,6 +27,9 @@
# [START example_cluster_policy_rule]
def task_must_have_owners(task: BaseOperator):
+ if task.owner and not isinstance(task.owner, str):
+ raise AirflowClusterPolicyViolation(f'''owner should be a string. Current value: {task.owner}''')
Review Comment:
Thanks, used repr value.
--
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] mik-laj commented on a diff in pull request #23359: Validate DAG owner to be a string.
Posted by GitBox <gi...@apache.org>.
mik-laj commented on code in PR #23359:
URL: https://github.com/apache/airflow/pull/23359#discussion_r862001051
##########
tests/cluster_policies/__init__.py:
##########
@@ -27,6 +27,9 @@
# [START example_cluster_policy_rule]
def task_must_have_owners(task: BaseOperator):
+ if task.owner and not isinstance(task.owner, str):
+ raise AirflowClusterPolicyViolation(f'''owner should be a string. Current value: {task.owner}''')
Review Comment:
In some cases, `str()` returns the output, which might look like a valid string when it really is a complex object, so we should use `repr()` to avoid confusion.
```
>>> f"Value: {datetime.datetime.now()}"
'Value: 2022-04-29 19:19:05.618692'
>>> f"Value: {datetime.datetime.now()!r}"
'Value: datetime.datetime(2022, 4, 29, 19, 19, 8, 385189)'
```
--
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] kaxil merged pull request #23359: Validate DAG owner to be a string.
Posted by GitBox <gi...@apache.org>.
kaxil merged PR #23359:
URL: https://github.com/apache/airflow/pull/23359
--
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] github-actions[bot] commented on pull request #23359: Validate DAG owner to be a string.
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #23359:
URL: https://github.com/apache/airflow/pull/23359#issuecomment-1113972066
The PR most likely needs to run full matrix of tests because it modifies parts of the core of Airflow. However, committers might decide to merge it quickly and take the risk. If they don't merge it quickly - please rebase it to the latest main at your convenience, or amend the last commit of the PR, and push it with --force-with-lease.
--
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