Skip to content

Conversation

@amustaque97
Copy link
Contributor

Closes: #1863

@github-actions
Copy link

github-actions bot commented Dec 4, 2025

Dry-run check results

[WARN  sync_team] sync-team is running in dry mode, no changes will be applied.
[INFO  sync_team] synchronizing crates-io
[INFO  sync_team] synchronizing github

@jieyouxu jieyouxu added needs-infra-admin-review This change requires one of the `infra-admins` to review. S-waiting-on-review Status: waiting on review from a team/WG/PG lead, an infra-admin, and/or a team-repo-admin. S-waiting-on-author Status: waiting on PR author and removed S-waiting-on-review Status: waiting on review from a team/WG/PG lead, an infra-admin, and/or a team-repo-admin. labels Dec 7, 2025
@amustaque97 amustaque97 force-pushed the feat-1863-detect-non-tracked-repos branch from 6b92402 to 2ad6262 Compare December 7, 2025 06:52
@amustaque97 amustaque97 force-pushed the feat-1863-detect-non-tracked-repos branch from 417df2c to c1ab92d Compare December 7, 2025 07:07
@amustaque97 amustaque97 requested a review from marcoieni December 7, 2025 07:09
@amustaque97
Copy link
Contributor Author

I have resolved all the comments, synced with the latest main branch and squashed commits. PR is ready for review 🙌🏻

@Kobzol
Copy link
Member

Kobzol commented Dec 11, 2025

I'm kinda confused why does this needs any permissions at all? We can just read the publicly available list of repositories without any GitHub token, right? I don't think that we need to even read private repositories; in fact this action could leak the existence of said repositories when it runs on CI (sometimes private repos can be created manually, I think, e.g. for security responses, I don't even think that team currently tracks all private repositories).

That would also allow this CI workflow run completely unprivileged, which would be better in general.

By the way, this will probably fail (at least) for the gll repository, which is still untracked.

@amustaque97
Copy link
Contributor Author

I'm kinda confused why does this needs any permissions at all? We can just read the publicly available list of repositories without any GitHub token, right? I don't think that we need to even read private repositories; in fact this action could leak the existence of said repositories when it runs on CI (sometimes private repos can be created manually, I think, e.g. for security responses, I don't even think that team currently tracks all private repositories).

That would also allow this CI workflow run completely unprivileged, which would be better in general.

By the way, this will probably fail (at least) for the gll repository, which is still untracked.

Interesting, thank you for saving us from a leak. I never realised until now that we only manage public repos using team repos. I'm glad we found it soon. I have refactored the code to use the public api.

Can you please take a second look?

Copy link
Member

@Kobzol Kobzol left a comment

Choose a reason for hiding this comment

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

It looks great, thank you!

We should eventually just switch to octocrab for the simple stuff, or at least unify the team and sync-team GitHub APIs, it's annoying to reimplement the same GitHub endpoints again and again :D

src/ci.rs Outdated
.iter()
.filter(|(org, repo)| {
// Skip archived repos
if repo.archived {
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we need to skip archived repositories, we should also have those in team.

@Kobzol Kobzol mentioned this pull request Dec 12, 2025
@Kobzol
Copy link
Member

Kobzol commented Dec 12, 2025

Opened #2180 to unblock this.

@marcoieni
Copy link
Member

marcoieni commented Dec 12, 2025

It looks great, thank you!

We should eventually just switch to octocrab for the simple stuff, or at least unify the team and sync-team GitHub APIs, it's annoying to reimplement the same GitHub endpoints again and again :D

Let's create our own common crate and not depend on octocrab, so that:

  • we have full control of the http client and we can implement better error handling, retries, etc
  • we minimize dependencies

At least this is my preference. Because for example we might want to use graphql for certain calls.

We could create a common crate later as a separate issue

@Kobzol
Copy link
Member

Kobzol commented Dec 12, 2025

So, I agree that we should definitely have our own interface (and use that in both team and sync-team). But inside that interface, we can just offload the simpler things to octocrab, even using the same shared HTTP client. The beauty of octocrab is that it allows you to do even fully custom REST API calls, and even GraphQL calls (we do that in bors), using the same authenticated client. So you can use octocrab's code for the simple stuff, but then still do more advanced stuff manually. And you can also reuse (but also easily extend!) its serde types for GitHub REST API datatypes. It's a win-win, IMO.

@marcoieni
Copy link
Member

Can you customize it with middlewares? E.g. retry automatically on every 500?

@Kobzol
Copy link
Member

Kobzol commented Dec 12, 2025

I think that you can configure it using any tower middleware (e.g. https://docs.rs/octocrab/latest/octocrab/struct.OctocrabBuilder.html#method.with_service). I'll try it.

EDIT: I forgot that octocrab is async. That's not worth the rewrite, IMO. Nevermind.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs-infra-admin-review This change requires one of the `infra-admins` to review. S-waiting-on-author Status: waiting on PR author

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Detect when a repo is not tracked

4 participants