Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

git_hosting_providers: Add support for Chromium repositories #24881

Merged
merged 7 commits into from
Feb 21, 2025

Conversation

hferreiro
Copy link
Contributor

@hferreiro hferreiro commented Feb 14, 2025

Add an implementation of GitHostingProvider for repositories hosted on https://chromium.googlesource.com. Pull requests target the Gerrit instance at https://chromium-review.googlesource.com and avatar images are fetched using the Gerrit REST API.

Screenshot 2025-02-20 at 6 43 37 PM Screenshot 2025-02-20 at 6 43 51 PM

Release Notes:

  • Added support for repositories hosted on chromium.googlesource.com for Git blames and permalinks.

Add an implementation of GitHostingProvider for repositories hosted on
https://chromium.googlesource.com. Pull requests target the Gerrit instance at
https://chromium-review.googlesource.com and avatar images are fetched using the
Gerrit REST API.

Release Notes:

- Support repositories hosted on https://chromium.googlesource.com
@cla-bot cla-bot bot added the cla-signed The user has signed the Contributor License Agreement label Feb 14, 2025
@hferreiro
Copy link
Contributor Author

@maxdeviant hey! This PR just has one thing missing. The avatar url at https://github.com/zed-industries/zed/pull/24881/files#diff-0296a250f68527abab595c466384867f70c35cf6052ac77a281fe72941654b27R187 doesn't return an image but an HTTP 302 that points to the actual image. Can you give a hint on how to make that work?

@maxdeviant maxdeviant self-assigned this Feb 14, 2025
@maxdeviant maxdeviant marked this pull request as ready for review February 20, 2025 23:44
@maxdeviant
Copy link
Member

@maxdeviant hey! This PR just has one thing missing. The avatar url at https://github.com/zed-industries/zed/pull/24881/files#diff-0296a250f68527abab595c466384867f70c35cf6052ac77a281fe72941654b27R187 doesn't return an image but an HTTP 302 that points to the actual image. Can you give a hint on how to make that work?

The avatar URLs seem to load for me:

Screenshot 2025-02-20 at 6 43 37 PM

I think we can go ahead and merge this.

@maxdeviant maxdeviant changed the title Add a git hosting provider for Chromium git_hosting_providers: Add support for Chromium repositories Feb 20, 2025
@hferreiro
Copy link
Contributor Author

Yeah, for some reason it didn't work before. Maybe it took some time to load, and I just gave up 🤷

@maxdeviant
Copy link
Member

Yeah, for some reason it didn't work before. Maybe it took some time to load, and I just gave up 🤷

While debugging it I noticed two things:

  1. In the repositories I was testing with, some of the older commits had dead links, which meant we couldn't retrieve the author information to construct the avatar URL.
  2. It did take a few seconds to load in the first time.

Copy link
Member

@maxdeviant maxdeviant left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

@maxdeviant maxdeviant merged commit ec00fb9 into zed-industries:main Feb 21, 2025
12 checks passed
@hferreiro
Copy link
Contributor Author

In the repositories I was testing with, some of the older commits had dead links

Do you have some of those at hand?

@maxdeviant
Copy link
Member

In the repositories I was testing with, some of the older commits had dead links

Do you have some of those at hand?

Hmm, odd, I'm not seeing the dead links anymore!

@hferreiro
Copy link
Contributor Author

hferreiro commented Feb 21, 2025

What I had more doubts about this was to either rely on Reviewed-On or Change-Id. The latter should be more reliable and other Gerrit projects not hosted at https://chromium.googlesource.com/ only use that but the issue with it is that multiple "PRs" can have the same Change-Id.

I can revisit this in the future though.

@hferreiro hferreiro deleted the chromium-hosting-provider branch February 21, 2025 09:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla-signed The user has signed the Contributor License Agreement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants