github: move all GitHub logic to dedicated class (bug 1996507, bug 1995679)

[shtrom]

• add a GitHub object allowing to renew tokens, parse URLs, and generate authenticated URLs
• also add a pre-authenticated session attribute to the GitHubAPI object
• utils: move URL_USERINFO_RE to const (bug 1995679)
• utils: add basic test for github classes

[zzzeid]

See #536 (comment). I think it would simplify things if we did much of this in the Repo class. We could simply pass the repo name and owner directly to this API class and avoid doing any parsing here.

If we need the repo URL we can generate it manually with this info.

Side note, the Repo class already has a method to normalize the URL (Repo.normalized_url and also Repo._github_repo_url).

[shtrom]

I agree that passing the owner and repo name strings is better than passing a Repo, and achieves the initial decoupling objective.

However, I think it may be better to do all the bespoke URL parsing in the GitHub class, so we have all the GitHub logic there. If we just pass a URL with no additional parsing needed, we can hide all the inner workings of GitHub in one location.

This also opens up the way for... supporting another hosting platform, where we could have HostingPlatformAPI.supports_url(url) , much like how GitHubAPI.is_github_url (we could acctually rename that method to match) is used to simplify the bespoke logic in here https://github.com/mozilla-conduit/lando/pull/604/files#diff-2337f63ffc234b7c7cbd13e6ab6f4e1122832768b9ea5250508f5830a5bf71e5L111-R109 This is obviously not needed at the moment, but it would make it easier to handle if ever needed.

I guess the flip side to my argument is if we need the repo_owner and repo_name anywhere outside of the GitHubAPI interaction. I suspect it's not the case, but I may be missing something.

[shtrom]

Ah, with the GitHub class, we can now get them out if needed:

github_repo = GitHub(<URL>)
github_repo.repo_owner
github_repo.repo_name

added utils.github.GitHub class for common URL and token logic

[shtrom]

@zzzeid I had to rebase, as it applied orders of magnitude more cleanly than merging the base branch back in.

There is just a single commit added 064c7da to add the GitHub class, which the GitHubAPI now inherits from.

[zzzeid]

@zzzeid I had to rebase, as it applied orders of magnitude more cleanly than merging the base branch back in.

There is just a single commit added 064c7da to add the GitHub class, which the GitHubAPI now inherits from.

I'm going to re-apply the two commits in this branch to the new base (i.e., c1cda56 and 064c7da).

added utils.const for URL_USERINFO_RE, and updated PR description

[shtrom]

Ok, @zzzeid this has been rebased onto the latest pilot PR, and should be good to go.

[zzzeid]

This will break all the spots that use the GitHubAPIClient class, so they need to be updated with the new implementation:

• src/lando/ui/pull_requests.py
• src/lando/api/legacy/workers/landing_worker.py
• src/lando/api/views.py

Also I think the PR description is outdated.

fixed callsites, and added some basic tests for good measure (it's the third PR where I start a test_github.py from scratch 😅); they allowed me to identify some discrepancies in behaviour, which I've fixed, too

[shtrom]

I reviewed the PR description, and I think it's still accurate. Do you think something's missing, perhaps? [I just added a mention of the tests]

[shtrom]

I had to cherry-pick the tests fix from main. This tagged along.

[zzzeid]

May have been a little easier to update the pilot branch with main and then bubble up the changes to all the branches. It's gonna be a little tricky approving things in a state that's different than what they will end up in the squash and merge.

[shtrom]

Yep fair enough. I didn't want to update the pilot branch myself, so as not to step of each other's feet.

[zzzeid]

r+ with some caveats/nits that should be addressed before merging.

[zzzeid]

May have been a little easier to update the pilot branch with main and then bubble up the changes to all the branches. It's gonna be a little tricky approving things in a state that's different than what they will end up in the squash and merge.

[zzzeid]

Same here (i.e., duplicated from Repo).

Also, i think these lines are mixing the concepts of a repo name and a repo URL. It's OK to have .git suffix in the repo URL (as long as it is so consistently) but not in the name.

The rest of Lando expects a GitHub repo URL to contain a .git suffix. It's the "normalized" URL that does not have a suffix (in terms of existing conventions/terminology.) I.e., Repo.url has a .git suffix, Repo.normalized_url does not.

[shtrom]

Yep, I had trouble supporting all URLs that we might throw at the util and doing the right thing, which ended up like that.

I just tried a bit harder, and found a way to make the RE as greedy as needed, and now it seems to work in all cases, regardless of whether the URL is normalised or not, or if it has a full REST path vs. just being a base URL. It also simplifies the logic! (:

[zzzeid]

Should this fail more loudly? If we're expecting credentials in the URL and we can't get them, maybe this should throw an exception instead.

[shtrom]

I think if it doesn't fail here, it will fail when trying to push, and then the warning should give us a clue.

[zzzeid]

I think given that the purpose of this method is to return an authenticated URL, not returning one should lead to a failure, rather than a warning. Won't block this but this could potentially obfuscate issues down the road.

[shtrom]

@zzzeid Ok, the merge was a bit bigger than I'd want to get through without review, but I think it looks good, and most of the noise is gone.

There's one question about early vs. late failure.

However, if you're happy with it, I'd say go ahead and merge it!

[zzzeid]

Looks good. I think the issue remaining can be addressed at a later time.

[zzzeid]

I think given that the purpose of this method is to return an authenticated URL, not returning one should lead to a failure, rather than a warning. Won't block this but this could potentially obfuscate issues down the road.