kaxil commented on code in PR #73741:
URL: https://github.com/apache/airflow/pull/73741#discussion_r4160913389
##########
task-sdk/src/airflow/sdk/definitions/dag.py:
##########
@@ -631,8 +631,13 @@ def _validate_catchup(self, _, catchup: bool):
@tags.validator
def _validate_tags(self, _, tags: Collection[str]):
- if tags and any(len(tag) > TAG_MAX_LEN for tag in tags):
- raise ValueError(f"tag cannot be longer than {TAG_MAX_LEN}
characters")
+ for tag in tags:
Review Comment:
`tags` is already a set by the time this runs (`_convert_tags` does
`set(tags or [])`), so when more than one tag is too long, which one gets named
depends on string hash order and can change between parses of the same file.
Iterating `sorted(tags)` would make it stable, or you could collect every
offending tag into the message so users can fix them all in one go.
##########
task-sdk/tests/task_sdk/definitions/test_dag.py:
##########
@@ -519,15 +519,18 @@ def test_invalid_type_for_args(attr: str, value: Any):
pytest.param(["a normal tag"], True, id="one tag"),
pytest.param(["a normal tag", "another normal tag"], True, id="two
tags"),
pytest.param(["a" * 100], True, id="a tag that's of just length 100"),
+ pytest.param(["a" * 100, "b" * 100], True, id="combined tag length
greater than 100"),
pytest.param(["a normal tag", "a" * 101], False, id="two tags and one
of them is of length > 100"),
],
)
def test__tags_length(tags: list[str], should_pass: bool):
if should_pass:
DAG("test-dag", schedule=None, tags=tags)
else:
- with pytest.raises(ValueError, match="tag cannot be longer than 100
characters"):
+ with pytest.raises(ValueError, match=r"101 characters.*100-character
limit") as exc_info:
DAG("test-dag", schedule=None, tags=tags)
+ message = str(exc_info.value)
+ assert f"{'a' * 30}..." in message
Review Comment:
This substring check still passes if the preview grows to 50 characters or
the whole 101-character tag ends up in the message, so it doesn't pin the
truncation. Matching the full message would, e.g. `match=re.escape(f"Dag tag
'{'a' * 30}...' is 101 characters, exceeding the 100-character limit")` (`re`
is already imported), and then the `exc_info` lines can go.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
