[GitHub] [airflow] ashb commented on a change in pull request #21798: Change BaseOperatorLink interface to take a ti_key, not a datetime

[GitBox <gi...@apache.org>]

ashb commented on a change in pull request #21798:
URL: https://github.com/apache/airflow/pull/21798#discussion_r814886878



##########
File path: airflow/models/abstractoperator.py
##########
@@ -239,19 +242,27 @@ def global_operator_extra_link_dict(self) -> Dict[str, Any]:
     def extra_links(self) -> List[str]:
         return list(set(self.operator_extra_link_dict).union(self.global_operator_extra_link_dict))
 
-    def get_extra_links(self, dttm: datetime.datetime, link_name: str) -> Optional[Dict[str, Any]]:
+    def get_extra_links(self, ti: "TaskInstance", link_name: str) -> Optional[str]:
         """For an operator, gets the URLs that the ``extra_links`` entry points to.
 
         :raise ValueError: The error message of a ValueError will be passed on through to
             the fronted to show up as a tooltip on the disabled link.
-        :param dttm: The datetime parsed execution date for the URL being searched for.
+        :param ti: The TaskInstance for the URL being searched for.
         :param link_name: The name of the link we're looking for the URL for. Should be
             one of the options specified in ``extra_links``.
         """
-        if link_name in self.operator_extra_link_dict:
-            return self.operator_extra_link_dict[link_name].get_link(self, dttm)
-        elif link_name in self.global_operator_extra_link_dict:
-            return self.global_operator_extra_link_dict[link_name].get_link(self, dttm)
+        link: Optional["BaseOperatorLink"] = self.operator_extra_link_dict.get(
+            link_name
+        ) or self.global_operator_extra_link_dict.get(link_name)
+        if not link:
+            return None
+        # Check for old function signature
+        parameters = inspect.signature(link.get_link).parameters
+        args = [name for name, p in parameters.items() if p.kind != p.VAR_KEYWORD]
+        if "ti_key" in args:

Review comment:
       @uranusjr Do you think tis is enough of a check, or should we _force_ `ti_key` to be accepted only as a keyword argument?
   
   Is it worth doing a `__init_subclass__` check on BaseOperatorLink to check the signature?

##########
File path: airflow/models/abstractoperator.py
##########
@@ -239,19 +242,27 @@ def global_operator_extra_link_dict(self) -> Dict[str, Any]:
     def extra_links(self) -> List[str]:
         return list(set(self.operator_extra_link_dict).union(self.global_operator_extra_link_dict))
 
-    def get_extra_links(self, dttm: datetime.datetime, link_name: str) -> Optional[Dict[str, Any]]:
+    def get_extra_links(self, ti: "TaskInstance", link_name: str) -> Optional[str]:
         """For an operator, gets the URLs that the ``extra_links`` entry points to.
 
         :raise ValueError: The error message of a ValueError will be passed on through to
             the fronted to show up as a tooltip on the disabled link.
-        :param dttm: The datetime parsed execution date for the URL being searched for.
+        :param ti: The TaskInstance for the URL being searched for.
         :param link_name: The name of the link we're looking for the URL for. Should be
             one of the options specified in ``extra_links``.
         """
-        if link_name in self.operator_extra_link_dict:
-            return self.operator_extra_link_dict[link_name].get_link(self, dttm)
-        elif link_name in self.global_operator_extra_link_dict:
-            return self.global_operator_extra_link_dict[link_name].get_link(self, dttm)
+        link: Optional["BaseOperatorLink"] = self.operator_extra_link_dict.get(
+            link_name
+        ) or self.global_operator_extra_link_dict.get(link_name)
+        if not link:
+            return None
+        # Check for old function signature
+        parameters = inspect.signature(link.get_link).parameters
+        args = [name for name, p in parameters.items() if p.kind != p.VAR_KEYWORD]
+        if "ti_key" in args:

Review comment:
       @uranusjr Do you think this is enough of a check, or should we _force_ `ti_key` to be accepted only as a keyword argument?
   
   Is it worth doing a `__init_subclass__` check on BaseOperatorLink to check the signature?




-- 
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