ephraimbuddy commented on code in PR #52876:
URL: https://github.com/apache/airflow/pull/52876#discussion_r2222154308


##########
providers/git/src/airflow/providers/git/bundles/git.py:
##########
@@ -218,32 +217,53 @@ def _convert_git_ssh_url_to_https(url: str) -> str:
         return f"{domain}/{repo_path}"
 
     def view_url(self, version: str | None = None) -> str | None:
+        """
+        Return a URL for viewing the DAGs in the repository.
+
+        This method is deprecated and will be removed in a future release. Use 
`view_url_template` instead.
+        """
         if not version:
             return None
-        url = self.repo_url
-        if not url:
+        template = self.view_url_template()
+        if not template:
             return None
+        return template.format(version=version)
+
+    def view_url_template(self) -> str | None:
+        if hasattr(self, "_view_url_template") and self._view_url_template:
+            # Backward compatibility for released Airflow versions
+            return self._view_url_template
+
+        if not self.repo_url:
+            return None
+
+        url = self.repo_url
         if url.startswith("git@"):
             url = self._convert_git_ssh_url_to_https(url)
         if url.endswith(".git"):
             url = url[:-4]
+
         parsed_url = urlparse(url)
         host = parsed_url.hostname
         if not host:
             return None
+
         if parsed_url.username or parsed_url.password:
             new_netloc = host
             if parsed_url.port:
                 new_netloc += f":{parsed_url.port}"
             url = parsed_url._replace(netloc=new_netloc).geturl()
+
         host_patterns = {
-            "github.com": f"{url}/tree/{version}",
-            "gitlab.com": f"{url}/-/tree/{version}",
-            "bitbucket.org": f"{url}/src/{version}",
+            "github.com": f"{url}/tree/{{version}}",
+            "gitlab.com": f"{url}/-/tree/{{version}}",
+            "bitbucket.org": f"{url}/src/{{version}}",
         }
-        if self.subdir:
-            host_patterns = {k: f"{v}/{self.subdir}" for k, v in 
host_patterns.items()}
+
+        # Add subdir placeholder if applicable
         for allowed_host, template in host_patterns.items():
             if host == allowed_host or host.endswith(f".{allowed_host}"):
+                if self.subdir:
+                    return f"{template}/{self.subdir}"

Review Comment:
   We could get wrong information if the version is resolved here. Because, the 
version changes and what's stored in the dagbundlemodel.version also changes 
which means the url will keep changing. What I did here was to resolve the url 
with dag_version.bundle version information when accessed from the API. That's 
why `render_url` is not using the version on the DagBundleModel but uses the 
supplied version which comes from dag_version.bundle_version



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

Reply via email to