fix: use correct upload url for github

[zckv]

Fix issue #658

The tests are perfectible.

[bernardcooke53]

I think the code is generally good, thank you for spotting the failed upload 🙏

Just wanted to ask a question around last_request in the tests, and also find out what issues you were facing with content-type assertions?

[bernardcooke53]

I think this is cleaner if you add a constant to the Github class

DEFAULT_ASSET_UPLOAD_DOMAIN = DEFAULT_API_DOMAIN.replace("api", "uploads")

saves doing the replace everywhere

[bernardcooke53]

This introduces a new source of errors that didn't exist before - what if you get a 404?

[bernardcooke53]

Here could also be simpler if you could do f"https://{domain}...".format(domain=Github.ASSET_UPLOAD_DOMAIN, ...)

[bernardcooke53]

What issues were you seeing with m.last_request? Aren't we examining the content of the last request regardless? Just curious why we need to take a copy

[bernardcooke53]

Not part of the changes you've made but you did highlight on the issue - perhaps the choice to exit 1 or not should be covered under --strict mode?

[zckv]

Thanks for reviewing my PR !

With ee99763, I used the decorator suppress_not_found for eventual 404, like in the get_release_id_by_tag() method.

I added a Github.DEFAULT_UPLOAD_DOMAIN and a self.upload_url for testing convenience. I did not use them in semantic_release/hvcs/github.py as the upload url is obtained with a GET request. Maybe the GET request can be removed, if in custom GitHub instances the upload url stay the same.

I added comments in test_github.py:620 and two asserts, so the problem is easier to view. I am not knowledgeable enough with Pytest to isolate and correct this; it seems like it is a bug from requests_mock.

[zckv]

Not part of the changes you've made but you did highlight on the issue - perhaps the choice to exit 1 or not should be covered under --strict mode?

As a user of the command "semantic release publish", I would like to have an exit code of 1 if the upload url is wrong and not any package is uploaded. So that the workflow is shown as failed. Maybe the --strict mode should differentiate between a 404 and a 422 on the POST request ?

[zckv]

This can be deleted if the m.last_request "bug" is found and corrected.

[bernardcooke53]

Hey @zckv - apologies for taking a while to come back to your comments, I've been sick for a couple of weeks and haven't really been able to look at this. I'll try and circle back shortly, re-review with your comments and finish this off

[zckv]

Hello @bernardcooke53,
I hope you are doing better. If further help is needed on this PR, I would like to try doing it if possible. And thank you for maintaining this helpful project.

[bernardcooke53]

Hey @zckv - just looked back over the PR and rebased it on some other changes, fixed linter errors with the new linter, etc. I found out what was happening with the last_request reference, let me try and summarise:

In your test code you had the following in the test:

test_upload_asset_succeeds

        ...
        assert (
            default_gh_client.upload_asset(
                release_id=mock_release_id,
                file=example_changelog_md.resolve(),
                label=label,
            )
            is True
        )
        assert m.called
        assert len(m.request_history) == 2
        post_req = m.last_request  # Reference of m.last_request is not fix
        assert isinstance(m.last_request, requests_mock.request._RequestObjectProxy)
        assert m.last_request.method == "POST"  # FIXME Response to PR 661
        assert (
            m.last_request.method == "POST"
        )  # last request does not seem to be a generator
        assert m.last_request.url == "{url}?{params}".format(
            url=default_gh_client.asset_upload_url(mock_release_id),
            params=urlencode(urlparams),
        )
        # There are two requests made
        # 1 - GET api.github.com -> get release info
        # 2 - POST uploads.github.com -> Uploads assets
        # call to m.last_request.url seems to change the m.last_request reference to the older request
        # A bug of requests_mock ?
        assert m.last_request.method == "GET"  # FIXME Response to PR 661
        # ^ You shall not pass

        # Check if content-type header was correctly set according to
        # mimetypes - not retesting guessing functionality
        assert {
            "Content-Type": mimetypes.guess_type(
                example_changelog_md.resolve(), strict=False
            )[0]
            or "application/octet-stream"
        }.items() <= post_req.headers.items()
        assert post_req.body == example_changelog_md.read_bytes()

But I think specifically because of this assertion (line 603):

        assert m.last_request.url == "{url}?{params}".format(
            url=default_gh_client.asset_upload_url(mock_release_id),
            params=urlencode(urlparams),
        )

You were calling default_gh_client.assert_upload_url in order to make the assertion, and that was changing the last_request reference on the mocker because a new GET request was being make 🙂

I changed the test to use request_history which should get around this issue - this example showed how you can use the history like a list of requests, which I thought worked quite well. What do you think of the code now?

[zckv]

The revisions from 53e27d4 seem fine, it's more readable.

Thanks for explaining the issue with default_gh_client.assert_upload_url !

I synced the fork with upstream commits, I don't know if that was necessary.

[bernardcooke53]

Thanks @zckv 🎉