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.
I think that some of the boilerplate above this line could be removed, but I'm not 100% of the implications.
ptamarit
force-pushed
the
set-alternate-zipball-url
branch
from
f637015 to
83e04f7
Compare
ptamarit
changed the title
ptamarit
force-pushed
the
set-alternate-zipball-url
branch
from
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.
just out of curiosity, is there any reason why it is not assigned before the assert (622)?
There was a problem hiding this comment.
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.
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.
Yes, it's a bit special:
- The
fetch_zipball_filefunction downloads the ZIP file based on therelease_zipball_urlfield. - We call
test_zipballin order to modify what therelease_zipball_urlfield will return before calling thefetch_zipball_filefunction.
I was going for a minimal fix, but I agree that it should be refactored.
There was a problem hiding this comment.
- 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_fileand 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: