potiuk commented on a change in pull request #17578:
URL: https://github.com/apache/airflow/pull/17578#discussion_r688306237
##########
File path: airflow/utils/task_group.py
##########
@@ -94,10 +95,15 @@ def __init__(
self.used_group_ids: Set[Optional[str]] = set()
self._parent_group = None
else:
- if not isinstance(group_id, str):
- raise ValueError("group_id must be str")
- if not group_id:
- raise ValueError("group_id must not be empty")
+ if prefix_group_id:
Review comment:
This is a good question. I think not.
If we do it - that's technically backwards incompatible change because some
DAGs that already used spaces might stop working. With prefix, those DAGs would
not work anyway so it's not breaking anything 'more'.
I do not think that there is any limitation in case we do not use group id
as prefix in task Id. I think it would work and it has nice properties. Spaces
and national characters especially might be much better in the name of the
group in the UI. We do not have separate 'display name' there and the Id is
displayed in the UI, so I'm 100% sure people will use their national characters
there (and spaces).
If we find that it does not work with those characters, i think we should
rather fix it than prevent it.
Now when we talk about it - maybe even the right fix should be to remove all
the invalid characters from group I'd rather than prevent it even in prefix
case ?
This one can make it to 2.1.3 but then in 2.2 we could add it as a feature
to allow such cases.
WDYT?
--
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-unsubscr...@airflow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
