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

treewide: fix unexpected nixpkgs data scheme breakage #272199

Merged
merged 3 commits into from
Dec 9, 2023
Merged

Conversation

RaitoBezarius
Copy link
Member

@RaitoBezarius RaitoBezarius commented Dec 5, 2023

Description of changes

This fixes various data integrity violations of reasonable expected schemes in nixpkgs.

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 24.05 Release Notes (or backporting 23.05 and 23.11 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

This is the only team in nixpkgs that unfortunately broke the maintainers schema.
github/githubId was missing.
github/githubId was missing.
@RaitoBezarius RaitoBezarius changed the title treewide(maintainers): fix team maintainers for ororatech treewide: fix unexpected nixpkgs data scheme breakage Dec 5, 2023
@06kellyjac
Copy link
Member

Nice fix. Would it make sense to add proper validation/typesafety for meta? Or is that a quite significant overhaul?

@RaitoBezarius
Copy link
Member Author

I think that's doable but can introduce perf penalty, we have ofborg for that and checkMeta

@06kellyjac
Copy link
Member

Ah ok. So that should for the most part avoid this in the future? Is checkMeta blocking or just advisory?

@RaitoBezarius
Copy link
Member Author

Ah ok. So that should for the most part avoid this in the future? Is checkMeta blocking or just advisory?

It's "blocking" from OfBorg perspective but in GitHub, you can bypass it I guess.

maintainers = [ teams.ororatech ];
maintainers = teams.ororatech.members;
Copy link
Member

@Artturin Artturin Dec 5, 2023

Choose a reason for hiding this comment

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

checkMeta checks for a list of attrs, not a list of maintainer entries.

There's a todo to use the maintainer type

maintainers = listOf (attrsOf anything); # TODO use the maintainer type from lib/tests/maintainer-module.nix

@RaitoBezarius RaitoBezarius marked this pull request as ready for review December 9, 2023 13:06
@RaitoBezarius
Copy link
Member Author

I will send this in the meantime to stop the evaluation outputting violation of the schemas.

@RaitoBezarius RaitoBezarius merged commit 676285f into master Dec 9, 2023
22 checks passed
@RaitoBezarius RaitoBezarius deleted the data-fixups branch December 9, 2023 13:07
Copy link
Contributor

github-actions bot commented Dec 9, 2023

Successfully created backport PR for release-23.11:

@zeuner
Copy link
Contributor

zeuner commented Dec 9, 2023

Maintainers not entering data into every optional (see https://github.com/NixOS/nixpkgs/blob/master/maintainers/maintainer-list.nix#L7) field is not a "data scheme breakage", but an application of Europe's GDPR law and similar legislation in other countries. Please be absolutely sure you have the consent of the affected people before adding personally identifying information into a public repository.

@RaitoBezarius
Copy link
Member Author

Maintainers not entering data into every optional (see https://github.com/NixOS/nixpkgs/blob/master/maintainers/maintainer-list.nix#L7) field is not a "data scheme breakage", but an application of Europe's GDPR law and similar legislation in other countries. Please be absolutely sure you have the consent of the affected people before adding personally identifying information into a public repository.

I am surprised that you are employing GDPR law towards that mean, your information was publicly available in the commit history.

If it's not possible for you to add your GitHub account which you are using to contribute, I'd advise considering contributing by sending patches via email to another contributor who can pick them up and removing yourself from maintainers as it makes it quite difficult to integrate you.

If this was an optional field, I can send the changes to make it mandatory and uniform.

@zeuner
Copy link
Contributor

zeuner commented Dec 9, 2023

I am surprised that you are employing GDPR law towards that mean, your information was publicly available in the commit history.

GDPR is exactly about making sure that information found somewhere will not be arbitrary used an different ways not expected and/or wanted by the person that published them. I'm not sure why nixpkgs explicitly refers to the GDPR (https://github.com/NixOS/nixpkgs/blob/master/maintainers/maintainer-list.nix#L52) if there is strong aversion within the maintainers against actually following it.

If it's not possible for you to add your GitHub account which you are using to contribute, I'd advise considering contributing by sending patches via email to another contributor who can pick them up and removing yourself from maintainers as it makes it quite difficult to integrate you.

I'm surprised to see nixpkgs would want to make contributions harder for someone who likes to send in bugfixes etc.

When my entry was added, I explicitly stated that I want to keep control over how to contact me in context of maintainership. I received positive feedback about that and it was added. So it's surprising that there is now a rather strong urge to surpass what was consented to earlier.

I can offer an (also optional) Matrix handle if it helps.

If this was an optional field, I can send the changes to make it mandatory and uniform.

I guess the people who decided about making everything apart from the name optional had their reasons for it, so why change it? It's not an improvement to increase dependence on GitHub, so it was a very reasonable decision to allow maintainership even for those who plan to move away from that platform.

@zeuner
Copy link
Contributor

zeuner commented Dec 9, 2023

@ncfavier As you defined to what extent the fields are optional (https://github.com/NixOS/nixpkgs/blob/master/maintainers/maintainer-list.nix#L7), could you clarify the intended usage of the maintainer fields? @RaitoBezarius basically asks me to stop doing PRs, even though I usually get rather positive feedback regarding my contributions. This seems to be rooted in the maintainer record data scheme. Personally, I saw the status quo as being very reasonable as it leaves maintainers with a high level of control over their personal data, which seems to be important if we want to leave the door open to contributors from different jurisdictions.

@RaitoBezarius
Copy link
Member Author

Just to be clear, I am not asking to stop sending PRs, I am asking that if a reasonable github field is available, that it is filled.

As I am not able to find in the commit history the merge commit of your introduction as a maintainer (improper squash and merge?), I am not even able to read the prior discussion on the matter.

Either case, not having your GitHub when it clearly exist makes any automation relying on this field impossible to build.

We can conclude that we should leave this field optional but I cannot make any promise that any automation will take into account the special case that you are in all of nixpkgs, hence, why I recommended to consider stepping out of maintenance.

I don't like GitHub either but this is the collaboration platform, if you don't want to participate socially in this, this will make everyone's life harder every time we need to reach you out. I am not in favor of that.

@ncfavier
Copy link
Member

ncfavier commented Dec 9, 2023

I only made email optional; GitHub was and still is optional. Your information shouldn't have been included without at least telling you.

@ncfavier
Copy link
Member

ncfavier commented Dec 9, 2023

And @tomkoid.

@zeuner
Copy link
Contributor

zeuner commented Dec 9, 2023

@RaitoBezarius

Well, to me "contributing by sending patches via email" appeared to be synonymous to "not doing PRs" directly. But even if the whole thing appeared rather rude at first sight (personal data record being changed without prior notice, let alon consent, threat to make the fields mandatory), I do see your point and I'm glad to be able to discuss this in a less polarized manner.

In case it matters, I looked up the conversation regarding my inclusion in the file: #259915 (comment) . Not sure whether I wrongly squashed or merged something back then.

My point is: Yes, this is the collaboration platform currently, but whether it remains that way should depend on what the contributors want. I actually considered the whole Nix/NixOS/Nixpkgs ecosystem to be a rather promising example of how to at least minimize vendor lockin issues: E.g., we have automation that can notify between Discourse and GitHub, which is one step towards being able to contribute without being a GitHub customer (BTW, why is there no Discourse handle field in maintainer-list.nix?). Why not move forward from there? If it doesn't currently do so, why not extend the existing automation to notify maintainers through e-mail, Matrix or Discourse? I might look into implementing that once I figure out how to build a local test setup for the automation, considering that this one of the things that excite me about this ecosystem.

I understand that some automation might still work sub-par without allowing GitHub to be used and still decided to contribute, and I'll also be glad to offer maintainership for more packages I grasp well. In my experience, people were and are able to reach me, and if I see how to make it easier without consenting to data processing I don't like, I'll gladly do so.

@RaitoBezarius
Copy link
Member Author

I only made email optional; GitHub was and still is optional. Your information shouldn't have been included without at least telling you.

I agree and I apologize sincerely for this, this is my bad.

@RaitoBezarius
Copy link
Member Author

@RaitoBezarius

Well, to me "contributing by sending patches via email" appeared to be synonymous to "not doing PRs" directly. But even if the whole thing appeared rather rude at first sight (personal data record being changed without prior notice, let alon consent, threat to make the fields mandatory), I do see your point and I'm glad to be able to discuss this in a less polarized manner.

In case it matters, I looked up the conversation regarding my inclusion in the file: #259915 (comment) . Not sure whether I wrongly squashed or merged something back then.

My point is: Yes, this is the collaboration platform currently, but whether it remains that way should depend on what the contributors want. I actually considered the whole Nix/NixOS/Nixpkgs ecosystem to be a rather promising example of how to at least minimize vendor lockin issues: E.g., we have automation that can notify between Discourse and GitHub, which is one step towards being able to contribute without being a GitHub customer (BTW, why is there no Discourse handle field in maintainer-list.nix?). Why not move forward from there? If it doesn't currently do so, why not extend the existing automation to notify maintainers through e-mail, Matrix or Discourse? I might look into implementing that once I figure out how to build a local test setup for the automation, considering that this one of the things that excite me about this ecosystem.

I understand that some automation might still work sub-par without allowing GitHub to be used and still decided to contribute, and I'll also be glad to offer maintainership for more packages I grasp well. In my experience, people were and are able to reach me, and if I see how to make it easier without consenting to data processing I don't like, I'll gladly do so.

Sorry for the tension, I felt also quite threatened by the GDPR argument which felt to me quite orthogonal as it didn't appear to me that your profile was PII per se. Nonetheless, putting this aside.

I have suggested multiple times to work on independence efforts but all of that takes time, I think I do my fair share of that. Nevertheless, we only have finite time and energy. I don't want to get my energy drained in endless discussions on the ideal move to a better platform, especially when it's only discussions, I do prefer actions.

Either case, I am working on pinging maintainers of security concerns and for this, the authentication has to go through GitHub (we don't have another silo of users, no LDAP, nothing.), I cannot really do anything about it, we don't have another ecosystem to authenticate people and maintainer-list is the only anchor for the permissions, in the absence of a link, I will simply not offer authentication to those users on the platform and disable all potential security notifications. I was under the hypothesis that this would be too much of a deal breaker compared to adding a GitHub handle which can be clearly found inside the commit history and for which I believed was just an overlook as I was not able to pinpoint the original PR.

Anyhow, I present my sincere apologies to you and potentially @tomkoid who may also disagree on this addition.

@zeuner
Copy link
Contributor

zeuner commented Dec 9, 2023

I have suggested multiple times to work on independence efforts but all of that takes time, I think I do my fair share of that.

Well, happy to help on that. We don't need yet another promising OS to slowly stagger towards proprietaryness.

@infinisil
Copy link
Member

infinisil commented Dec 10, 2023

I opened #273332 to revert the unnecessary changes from this PR. Edit: Merged, let's discuss the policy change in #273220

infinisil added a commit that referenced this pull request Dec 10, 2023
This reverts commit 3429e79.

GitHub names/ids are not mandatory for now. See
#273220,
#272199 and
#273146.

Furthermore this user has not consented to their account information being added
to the listing. While we can't reasonably change the git history, we can
revert the change so it won't be included in future revisions.
infinisil added a commit that referenced this pull request Dec 10, 2023
This reverts commit bc0adb2.

GitHub names/ids are not mandatory for now. See
#273220,
#272199 and
#273146.

Furthermore this user has not consented to their account information being added
to the listing. While we can't reasonably change the git history, we can
revert the change so it won't be included in future revisions.
@jpds
Copy link
Contributor

jpds commented Dec 10, 2023

I'm not sure why nixpkgs explicitly refers to the GDPR (https://github.com/NixOS/nixpkgs/blob/master/maintainers/maintainer-list.nix#L52) if there is strong aversion within the maintainers against actually following it.

FWIW, this was added for a very different reason than what you're arguing here: f13283c

@zeuner
Copy link
Contributor

zeuner commented Dec 11, 2023

I'm not sure why nixpkgs explicitly refers to the GDPR (https://github.com/NixOS/nixpkgs/blob/master/maintainers/maintainer-list.nix#L52) if there is strong aversion within the maintainers against actually following it.

FWIW, this was added for a very different reason than what you're arguing here: f13283c

I'm aware of that. Still, it's misleading to claim doing something to comply with the GDPR when the very same file is maintained under outright denial of the GDPR.

dansbandit pushed a commit to dansbandit/nixpkgs that referenced this pull request Dec 27, 2023
This reverts commit 3429e79.

GitHub names/ids are not mandatory for now. See
NixOS#273220,
NixOS#272199 and
NixOS#273146.

Furthermore this user has not consented to their account information being added
to the listing. While we can't reasonably change the git history, we can
revert the change so it won't be included in future revisions.
dansbandit pushed a commit to dansbandit/nixpkgs that referenced this pull request Dec 27, 2023
This reverts commit bc0adb2.

GitHub names/ids are not mandatory for now. See
NixOS#273220,
NixOS#272199 and
NixOS#273146.

Furthermore this user has not consented to their account information being added
to the listing. While we can't reasonably change the git history, we can
revert the change so it won't be included in future revisions.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants