Search for GitLab PRs in Launchpad

[sergeibbb]

#3788

Description

Checklist

• I have followed the guidelines in the Contributing document
• My changes follow the coding style of this project
• My changes build without any errors or warnings
• My changes have been formatted and linted
• My changes include any required corresponding changes to the documentation (including CHANGELOG.md and README.md)
• My changes have been rebased and squashed to the minimal number (typically 1) of relevant commits
• My changes have a descriptive commit message with a short title, including a Fixes $XXX - or Closes #XXX - prefix to auto-close the issue that your PR addresses

[sergeibbb]

@axosoft-ramint

In the last commit I'm grouping detected PR-identity by providers, because thought that if we know that an URL matches to a GitLab then no need to search for it in GitHub. But if it looks too compicated, then then last commit can be rejected.

[axosoft-ramint]

As it stands, this doesn't quite feel ready. The utils and test files that are provider-specific should be organized into provider-specific directories. The url identity being a mapped object by provider id feels confusing - the identity itself should just come in on the function. Some of the changes to the PR utils need either explanation or rewrite to make more sense (why are we defining an undefined const and then returning it without touching it?). The same function being defined in the PR utils but then also implemented specifically for integrations doesn't quite make sense to me. The checkoutable logic in launchpad shouldn't be necessary - we should always have at least a defined head ref and remote url for a PR, or else we're not fetching enough info.

Can talk on this further on your return.

[CbtSlave]

gh pr checkout 3795

[axosoft-ramint]

On testing, the only issue I'm seeing is that owner/repo are not showing up on GitLab search results:

They do for GitLab PRs that are not search results.

Apart from that, some minor nitpicks that should be easy to change:

[axosoft-ramint]

This is unnecessary since we return the promise in the next line, similar to other cases. If anything, I would suggest instead that we just combine the two lines to return this.container.cache.get... and do the same in other places where this pattern occurs across the file.

[sergeibbb]

@axosoft-ramint

I did it because I set a breakpoint at this place it was convenient to see a resolved promise.

[axosoft-ramint]

Let's follow a similar pattern for generic and provider-specific function implementation and naming here as we do for other functions, in case we want to expand on the base logic later:

1. Have a protected getProviderPullRequestIdentityFromMaybeUrl that can be implemented by specific providers (i.e. GitHub, GitLab, etc.)
2. Have a getPullRequestIdentityFromMaybeUrl that, for now, just does return getProviderPullRequestIdentityFromMaybeUrl and later if we want, we can layer in caching there.

[axosoft-ramint]

I suggest calling this one getPullRequestIdentityFromSearch to avoid making it seem as if line 607 is a recursive call to the same function.