Skip to content

Add meta.teams support #914

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 5 commits into from
May 6, 2025
Merged

Add meta.teams support #914

merged 5 commits into from
May 6, 2025

Conversation

RossComputerGuy
Copy link
Member

Fixes #907

We should get this close with NixOS/nixpkgs#400458.

My lack of experience with rust got me some progress but I couldn't get past this error:

flake-info> error[E0277]: the trait bound `export::Maintainer: From<OneOrMany<import::Maintainer>>` is not satisfied
flake-info>    --> src/data/export.rs:387:42
flake-info>     |
flake-info> 387 |                 members: members.map(|m| Maintainer::from(m)),
flake-info>     |                                          ^^^^^^^^^^ the trait `From<OneOrMany<import::Maintainer>>` is not implemented for `export::Maintainer`
flake-info>     |
flake-info>     = help: the following other types implement trait `From<T>`:
flake-info>               `export::Maintainer` implements `From<flake::Flake>`
flake-info>               `export::Maintainer` implements `From<import::Maintainer>`
flake-info>
flake-info> error[E0308]: mismatched types
flake-info>    --> src/data/export.rs:387:26
flake-info>     |
flake-info> 387 |                 members: members.map(|m| Maintainer::from(m)),
flake-info>     |                          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected `Option<OneOrMany<Maintainer>>`, found `Option<Maintainer>`
flake-info>     |
flake-info>     = note: expected enum `std::option::Option<OneOrMany<export::Maintainer>>`
flake-info>                found enum `std::option::Option<export::Maintainer>`
flake-info>
flake-info> Some errors have detailed explanations: E0277, E0308.
flake-info> For more information about an error, try `rustc --explain E0277`.
flake-info> error: could not compile `flake-info` (lib) due to 2 previous errors

So whoever reviews this needs to know Rust. We need members to be a list because that's what it is in Nix. But it doesn't return OneOrMany applied to the mapped list.

@RossComputerGuy RossComputerGuy mentioned this pull request Apr 21, 2025
Merged
13 tasks
@RossComputerGuy RossComputerGuy force-pushed the feat/meta-teams branch 4 times, most recently from d8a17dd to ffec7f1 Compare April 22, 2025 02:50
@RossComputerGuy
Copy link
Member Author

I figured out a solution to the error.

raboof
raboof reviewed Apr 22, 2025
View reviewed changes
Copy link
Member

@raboof raboof left a comment

Choose a reason for hiding this comment

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

Intent is 👍 , one question about the code, haven't tested.

flake-info/src/data/export.rs Outdated
#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)]
#[allow(non_snake_case)]
pub struct Team {
members: Option<OneOrMany<Maintainer>>,
Copy link
Member

Choose a reason for hiding this comment

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

I'm not super familiar with this codebase yet, but I wonder if it wouldn't be simpler for this to be a Vec (or Option<Vec>) here?

Copy link
Member

Choose a reason for hiding this comment

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

Would be much simpler, yes. Specifically Option<OneOrMany<T>> sounds like a complicated way to specify Vec<T>. Perhaps it has benefits when (de-)serializing.

Copy link
Member Author

Choose a reason for hiding this comment

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

Option<OneOrMany<T>> is done in many other places so I used it because of that

Copy link
Member

Choose a reason for hiding this comment

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

I had the impression that it is used in the contract between the 'nix side' and 'flake-info', but not between 'flake-info' and elasticsearch model - and that this struct is used for the latter. As said I'm not super familiar with this codebase yet, though, so I could be mistaken.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I'm not sure either. I think it does everything lol.

Copy link
Member

Choose a reason for hiding this comment

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

is import.rs the 'nix side' and export.rs the 'elasticsearch side'?

my guess was that OneOrMany would support both "foo" and [ "foo" "bar" ] due to nix being flexibly typed, but we can be a bit more strict on our 'own' side.

Copy link
Member Author

Choose a reason for hiding this comment

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

I have no idea lol.

@RossComputerGuy
Copy link
Member Author

NixOS/nixpkgs#400458 was just merged, we should get this PR before it hits nixos-unstable and entries disappear from https://search.nixos.org.

@raboof
Copy link
Member

raboof commented Apr 26, 2025

I'm not a huge fan of merging this feature without figuring out whether that OneOrMany complexity is really needed there, but I suppose it's better than losing maintainer information.

How have you tested this change?

@raboof
Copy link
Member

raboof commented Apr 26, 2025

I tried testing this with the gitlab-container-registry package, but after uploading that package, the query generated by the frontend can't find it at all anymore. Could you have a look at that?

@RossComputerGuy
Copy link
Member Author

How have you tested this change?

I haven't tried testing it because idk how to test this and the README doesn't help.

@raboof
Copy link
Member

raboof commented Apr 26, 2025

Makes sense. I didn't either and spent this morning figuring it out, created #915 / #916 / #917 / #918

I'm afraid I probably won't find time to figure out why gitlab-container-registry no longer shows up for me after applying your change any time soon.

raboof added 2 commits May 5, 2025 14:46
@raboof
meta.teams support: fix export
7438a23
@raboof
meta.teams support: show team name
d8c1bc9 
so it works even for teams that don't have GitHub teams
@raboof
Copy link
Member

raboof commented May 6, 2025

The page now shows "This package has no maintainers. If you find it useful, please consider becoming a maintainer!" in the "Maintainers" section when the package has no individual maintainers but only teams - that might be something to fix as well?

@raboof
Copy link
Member

raboof commented May 6, 2025

I have proposed two fixes for this PR in RossComputerGuy#1 and RossComputerGuy#2

raboof
raboof approved these changes May 6, 2025
View reviewed changes
Copy link
Member

@raboof raboof left a comment

Choose a reason for hiding this comment

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

let's give this a try 🤞

@raboof raboof merged commit f8dec63 into NixOS:main May 6, 2025
3 checks passed
@RossComputerGuy RossComputerGuy deleted the feat/meta-teams branch May 6, 2025 19:13
@RossComputerGuy
Copy link
Member Author

Doesn't work, got this error: Expecting an OBJECT with a field named 'package_teams'

@raboof raboof mentioned this pull request May 6, 2025
Closed
@raboof
Copy link
Member

raboof commented May 6, 2025

can you PR an update changing the VERSION file in the root to '43'?

@RossComputerGuy
Copy link
Member Author

Oh, I can't right now... I can in about 6 hours.

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

@raboof raboof raboof approved these changes

+1 more reviewer

@FliegendeWurst FliegendeWurst FliegendeWurst left review comments

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Support for meta.teams
3 participants
@RossComputerGuy @raboof @FliegendeWurst