Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add view_url for DagBundles #45126

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

ephraimbuddy
Copy link
Contributor

This PR adds view_url to Dagbundles to enable viewing the bundle's version

@boring-cyborg boring-cyborg bot added the area:Scheduler including HA (high availability) scheduler label Dec 20, 2024
This PR adds view_url to Dagbundles to enable viewing the bundle's
version
@ephraimbuddy ephraimbuddy marked this pull request as ready for review December 21, 2024 19:02
Comment on lines 139 to 140
if not version:
raise AirflowException("Version is required to view the repository")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typing allowing none versions, but having it always raise, feels a bit awkward. Perhaps we just return none in this case?

Comment on lines 141 to 142
if not self._has_version(self.repo, version):
raise AirflowException(f"Version {version} not found in the repository")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure we should even check - we are just building a url, let the other side display a 404.

Comment on lines 131 to 136
if self.repo_url.startswith("git@"):
parts = self.repo_url.split(":")
domain = parts[0].replace("git@", "https://")
repo_path = parts[1].replace(".git", "")
return f"{domain}/{repo_path}"
raise ValueError(f"Invalid git SSH URL: {self.repo_url}")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if self.repo_url.startswith("git@"):
parts = self.repo_url.split(":")
domain = parts[0].replace("git@", "https://")
repo_path = parts[1].replace(".git", "")
return f"{domain}/{repo_path}"
raise ValueError(f"Invalid git SSH URL: {self.repo_url}")
if not self.repo_url.startswith("git@"):
raise ValueError(f"Invalid git SSH URL: {self.repo_url}")
parts = self.repo_url.split(":")
domain = parts[0].replace("git@", "https://")
repo_path = parts[1].replace(".git", "")
return f"{domain}/{repo_path}"

url = self._convert_git_ssh_url_to_https()
if url.endswith(".git"):
url = url[:-4]
if "github" in url:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if "github" in url:
if url.startswith("https://github.com"):

I wonder if this might be a bit more resilient?

url = self.repo_url
if url.startswith("git@"):
url = self._convert_git_ssh_url_to_https()
if url.startswith("https://github.com"):

Check failure

Code scanning / CodeQL

Incomplete URL substring sanitization High

The string
https://github.com
may be at an arbitrary position in the sanitized URL.
url = self._convert_git_ssh_url_to_https()
if url.startswith("https://github.com"):
return f"{url}/tree/{version}"
if url.startswith("https://gitlab.com"):

Check failure

Code scanning / CodeQL

Incomplete URL substring sanitization High

The string
https://gitlab.com
may be at an arbitrary position in the sanitized URL.
return f"{url}/tree/{version}"
if url.startswith("https://gitlab.com"):
return f"{url}/-/tree/{version}"
if url.startswith("https://bitbucket.org"):

Check failure

Code scanning / CodeQL

Incomplete URL substring sanitization High

The string
https://bitbucket.org
may be at an arbitrary position in the sanitized URL.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:Scheduler including HA (high availability) scheduler
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants