Skip to content

api: fix set alternate zipball URL when tag and branch having same name #173

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/workflows/tests.yml
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ jobs:
pip install ".[$EXTRAS]"
pip freeze
docker --version
docker-compose --version
docker compose --version

- name: Run translations test
run: ./run-i18n-tests.sh
Expand Down
5 changes: 4 additions & 1 deletion invenio_github/api.py
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
# -*- coding: utf-8 -*-
#
# This file is part of Invenio.
# Copyright (C) 2023 CERN.
# Copyright (C) 2023-2024 CERN.
#
# Invenio is free software; you can redistribute it
# and/or modify it under the terms of the GNU General Public License as
Expand Down Expand Up @@ -623,6 +623,9 @@ def test_zipball(self):
response.status_code == 200
), f"Could not retrieve archive from GitHub: {zipball_url}"

# Overwrite the original URL with the newly found more specific URL.
self.release_payload["zipball_url"] = zipball_url
Copy link
Contributor

Choose a reason for hiding this comment

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

just out of curiosity, is there any reason why it is not assigned before the assert (622)?

Copy link
Member Author

Choose a reason for hiding this comment

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

No particular reason, I was just thinking "if the status code is 200, then the URL is OK and can be stored".
But the assert would stop the code execution flow anyway, so it doesn't matter.

Copy link
Member

Choose a reason for hiding this comment

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

minor: I'm not sure if this is the best way to go about this, since we're effectively modifying the content of the webhook event we received from GitHub (temporarily in the fetched DB object, and I guess not actually persisting it in the DB).

Looking at this now, I'm somewhat confused/unhappy with how the whole function works. Shouldn't this just return the final/good Zipball URL? I would expect that this would be the result of fetch_zipball_file or release_zipball_url.

Copy link
Member Author

@ptamarit ptamarit Sep 10, 2024

Choose a reason for hiding this comment

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

Yes, it's a bit special:

  • The fetch_zipball_file function downloads the ZIP file based on the release_zipball_url field.
  • We call test_zipball in order to modify what the release_zipball_url field will return before calling the fetch_zipball_file function.

I was going for a minimal fix, but I agree that it should be refactored.

Copy link
Member Author

@ptamarit ptamarit Sep 16, 2024

Choose a reason for hiding this comment

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


# High level API

def release_failed(self):
Expand Down
15 changes: 13 additions & 2 deletions tests/conftest.py
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
# -*- coding: utf-8 -*-
#
# This file is part of Invenio.
# Copyright (C) 2023 CERN.
# Copyright (C) 2023-2024 CERN.
#
# Invenio is free software; you can redistribute it
# and/or modify it under the terms of the GNU General Public License as
Expand Down Expand Up @@ -286,10 +286,21 @@ def mock_repo_with_id(id):
def mock_repo_by_name(owner, name):
return repos_by_name.get("/".join((owner, name)))

def mock_head_status_by_repo_url(url, **kwargs):
url_specific_refs_tags = (
"https://github.com/auser/repo-2/zipball/refs/tags/v1.0-tag-and-branch"
)
if url.endswith("v1.0-tag-and-branch") and url != url_specific_refs_tags:
return MagicMock(
status_code=300, links={"alternate": {"url": url_specific_refs_tags}}
)
else:
return MagicMock(status_code=200)

mock_api.repository_with_id.side_effect = mock_repo_with_id
mock_api.repository.side_effect = mock_repo_by_name
mock_api.markdown.side_effect = lambda x: x
mock_api.session.head.return_value = MagicMock(status_code=200)
mock_api.session.head.side_effect = mock_head_status_by_repo_url
mock_api.session.get.return_value = MagicMock(raw=ZIPBALL())

with patch("invenio_github.api.GitHubAPI.api", new=mock_api):
Expand Down
42 changes: 41 additions & 1 deletion tests/test_api.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
# -*- coding: utf-8 -*-
#
# Copyright (C) 2023 CERN.
# Copyright (C) 2023-2024 CERN.
#
# Invenio-Github is free software; you can redistribute it and/or modify
# it under the terms of the MIT License; see LICENSE file for more details.
Expand Down Expand Up @@ -78,3 +78,43 @@ def test_release_api(app, test_user, github_api):

assert valid_remote_file_contents is not None
assert valid_remote_file_contents.decoded["name"] == "test.py"


def test_test_zipball(app, test_user, github_api):
api = GitHubAPI(test_user.id)
api.init_account()
repo_id = 2
repo_name = "repo-2"

# Create a repo hook
hook_created = api.create_hook(repo_id=repo_id, repo_name=repo_name)
assert hook_created

headers = [("Content-Type", "application/json")]

payload = github_payload_fixture(
"auser", repo_name, repo_id, tag="v1.0-tag-and-branch"
)
with app.test_request_context(headers=headers, data=json.dumps(payload)):
event = Event.create(
receiver_id="github",
user_id=test_user.id,
)
release = Release(
release_id=payload["release"]["id"],
tag=event.payload["release"]["tag_name"],
repository_id=repo_id,
event=event,
status=ReleaseStatus.RECEIVED,
)
# Idea is to test the public interface of GithubRelease
gh = GitHubRelease(release)
Copy link
Member Author

Choose a reason for hiding this comment

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

I think that some of the boilerplate above this line could be removed, but I'm not 100% of the implications.


# Call the method fixing the zipball URL
gh.test_zipball()

# Check that the zipball URL is the alternate URL specific for tags
assert (
gh.release_zipball_url
== "https://github.com/auser/repo-2/zipball/refs/tags/v1.0-tag-and-branch"
)
Loading