-
Notifications
You must be signed in to change notification settings - Fork 26
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
api: fix set alternate zipball URL when tag and branch having same name #173
base: master
Are you sure you want to change the base?
api: fix set alternate zipball URL when tag and branch having same name #173
Conversation
status=ReleaseStatus.RECEIVED, | ||
) | ||
# Idea is to test the public interface of GithubRelease | ||
gh = GitHubRelease(release) |
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.
I think that some of the boilerplate above this line could be removed, but I'm not 100% of the implications.
f637015
to
83e04f7
Compare
83e04f7
to
193e1b3
Compare
@@ -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 |
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.
just out of curiosity, is there any reason why it is not assigned before the assert (622)?
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.
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.
@@ -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 |
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.
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
.
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.
Yes, it's a bit special:
- The
fetch_zipball_file
function downloads the ZIP file based on therelease_zipball_url
field. - We call
test_zipball
in order to modify what therelease_zipball_url
field will return before calling thefetch_zipball_file
function.
I was going for a minimal fix, but I agree that it should be refactored.
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.
- Question: do we need the 2-step part of first validating that the release files are fetchable and later initializing files ?
- If the 2-steps are needed, then return the value here and use it in
invenio_rdm_records/services/github/release.py
- If the 2-steps are not needed, move all this method to
fetch_zipball_file
and make this method empty and deprecated.
- If the 2-steps are needed, then return the value here and use it in
❤️ Thank you for your contribution!
Description
Fixes #172
Checklist
Ticks in all boxes and 🟢 on all GitHub actions status checks are required to merge:
Frontend
Reminder
By using GitHub, you have already agreed to the GitHub’s Terms of Service including that: