Enhancements for ppm publish

[savetheclocktower]

Have you checked for existing feature requests?

• Completed

Summary

I found a few issues with the publish task, mainly around the part where we check GitHub to see whether it knows about the new tag:

1. The waitForTagToBeAvailable method assumes that the tag will eventually show up. It doesn't try to handle situations where the API returns errors — for example, because of an exceeded rate limit.
2. It doesn't try to handle situations where we check the API 5 times and give up. We just resolve as though the tag is ready, even though it's clearly not ready yet. We should probably fail here with a helpful error message like “make sure the tag is present.”
3. The exceeded rate limit is somewhat easy to trigger locally because it's 60 requests per hour for unauthenticated users. If waitForTagToBeAvailable used the same token that PPM uses in calls to the PPM API, that limit could be raised at least tenfold. It's worth doing.
4. The code that calls waitForTagToBeAvailable doesn't have any error processing of its own. It doesn't envision that waitForTagToBeAvailable can fail in any way. If we can't be sure the tag is present, we shouldn't proceed with publishing.

What benefits does this feature provide?

Better error handling for ppm publish.

Any alternatives?

Probably.

Other examples:

No response

[savetheclocktower]

Oh, I forgot one: there's no test coverage for this method because it makes requests to an external URL. We should either

1. add the ability for the user to point these requests to a different URL via environment variable (much like we can do the same for API calls to api.pulsar-edit.dev), or
2. change this method to make requests to an api.pulsar-edit.dev endpoint, even if all it does is proxy calls to the equivalent GitHub endpoint (thereby allowing us to mock server responses with the same technique we already use for package-backend calls).

Option 1 is probably easier, but maybe option 2 makes sense for other reasons that I'm unaware of.

[confused-Techie]

(Sorry I haven't reviewed the PR content) But to comment only on your ideas about better testing.

I'd be much more inclined to go with option 1.
Namely, because PPM already knows the GitHub repo to check, whereas if the call was made to the backend, it'd be via the package name, which then invokes a lookup to the database to find the repo, which can then be redirected to. At a glance this seems like a lot of system usage to help a testing only case, but if there's other benefits I'm unaware of I'd love to hear more.

Otherwise allowing customization of this could prove beneficial if we ever do move further on the conversation of more forge support (Although I have a feeling that'd involve way more due to API differences between services) but who knows could still maybe be a step in the right direction.

[confused-Techie]

@savetheclocktower Some ideas, I've recently been looking at this library for us in pulsar-edit/package-backend.

It hooks into the NodeJS built-in HTTP handlers to mock responses to any URL. If I recall correctly, superagent does in fact use the built in handlers at the base, so it should be compatible for both the backend repo and here.

So this may be something we could look at to implement mocking of GitHub endpoints right now, with zero other changes.

If you'd like I can first go about testing this over in the backend repo to hopefully encounter any of the pitfalls that we may find.

[savetheclocktower]

Sure, let me know what you find. Seems promising.

[DeeDeeG]

Unclear to me how many of these issues/pieces of feedback might be different now with the large refactor of how ppm publish is written in #132.

Just noting it in case it is relevant, leaving the breadcrumb trail, so to speak...