-
Notifications
You must be signed in to change notification settings - Fork 14.5k
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
base: main
Are you sure you want to change the base?
Add view_url for DagBundles #45126
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -126,3 +126,29 @@ def refresh(self) -> None: | |||||
|
||||||
self.bare_repo.remotes.origin.fetch("+refs/heads/*:refs/heads/*") | ||||||
self.repo.remotes.origin.pull() | ||||||
|
||||||
def _convert_git_ssh_url_to_https(self) -> str: | ||||||
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}") | ||||||
|
||||||
def view_url(self, version: str | None = None) -> str: | ||||||
if not version: | ||||||
raise AirflowException("Version is required to view the repository") | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? |
||||||
if not self._has_version(self.repo, version): | ||||||
raise AirflowException(f"Version {version} not found in the repository") | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||||||
url = self.repo_url | ||||||
if url.startswith("git@"): | ||||||
url = self._convert_git_ssh_url_to_https() | ||||||
if url.endswith(".git"): | ||||||
url = url[:-4] | ||||||
if "github" in url: | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
I wonder if this might be a bit more resilient? |
||||||
return f"{url}/tree/{version}" | ||||||
if "gitlab" in url: | ||||||
return f"{url}/-/tree/{version}" | ||||||
if "bitbucket" in url: | ||||||
return f"{url}/src/{version}" | ||||||
return f"{url}/tree/{version}" |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -19,6 +19,7 @@ | |
|
||
import tempfile | ||
from pathlib import Path | ||
from unittest import mock | ||
|
||
import pytest | ||
from git import Repo | ||
|
@@ -265,3 +266,49 @@ def test_subdir(self, git_repo): | |
files_in_repo = {f.name for f in bundle.path.iterdir() if f.is_file()} | ||
assert str(bundle.path).endswith(subdir) | ||
assert {"some_new_file.py"} == files_in_repo | ||
|
||
@pytest.mark.parametrize( | ||
"repo_url, expected_url", | ||
[ | ||
("[email protected]:apache/airflow.git", "https://github.com/apache/airflow/tree/0f0f0f"), | ||
("[email protected]:apache/airflow.git", "https://gitlab.com/apache/airflow/-/tree/0f0f0f"), | ||
("[email protected]:apache/airflow.git", "https://bitbucket.org/apache/airflow/src/0f0f0f"), | ||
], | ||
) | ||
@mock.patch("airflow.dag_processing.bundles.git.Repo") | ||
@mock.patch.object(GitDagBundle, "_has_version") | ||
def test_view_url(self, mock_has_version, mock_gitrepo, repo_url, expected_url): | ||
mock_has_version.return_value = True | ||
|
||
bundle = GitDagBundle( | ||
name="test", | ||
refresh_interval=300, | ||
repo_url=repo_url, | ||
tracking_ref="main", | ||
) | ||
view_url = bundle.view_url("0f0f0f") | ||
assert view_url == expected_url | ||
|
||
@mock.patch("airflow.dag_processing.bundles.git.Repo") | ||
def test_view_url_raises_if_no_version(self, mock_gitrepo): | ||
bundle = GitDagBundle( | ||
name="test", | ||
refresh_interval=300, | ||
repo_url="[email protected]:apache/airflow.git", | ||
tracking_ref="main", | ||
) | ||
with pytest.raises(AirflowException, match="Version is required to view the repository"): | ||
bundle.view_url(None) | ||
|
||
@mock.patch("airflow.dag_processing.bundles.git.Repo") | ||
@mock.patch.object(GitDagBundle, "_has_version") | ||
def test_view_url_raises_if_version_not_found(self, mock_has_version, mock_gitrepo): | ||
mock_has_version.return_value = False | ||
bundle = GitDagBundle( | ||
name="test", | ||
refresh_interval=300, | ||
repo_url="[email protected]:apache/airflow.git", | ||
tracking_ref="main", | ||
) | ||
with pytest.raises(AirflowException, match="Version not_found not found in the repository"): | ||
bundle.view_url("not_found") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.