Drift/pvv-nixos-config
Drift/pvv-nixos-config
16
4
Fork 1
Code Issues Pull Requests 6 Actions 1 Wiki Activity

bekkalokk/gitea/import-users: refactor + add members to groups #70

Merged
oysteikt merged 1 commits from gitea-import-users-add-to-members into main 2024-08-27 22:07:30 +02:00
Conversation 2 Commits 1 Files Changed 3 +163 -56
oysteikt commented 2024-08-26 21:44:14 +02:00
Owner
Copy Link

Fixes: Drift/issues#128

This refactors out most of the gitea API calls, so they are both easy to verify, and potentially switch out later if we need to.

It also adds all users to some common organization groups.

This required some new privileges for the token, so the token has been switched out as well.

It currently has:

  • admin read/write
  • organization read/write
  • repository read/write
  • user read/write

Not sure if everything is needed, but I think it's a fair subset. admin read/write was already required to create users.

Fixes: https://git.pvv.ntnu.no/Drift/issues/issues/128 This refactors out most of the gitea API calls, so they are both easy to verify, and potentially switch out later if we need to. It also adds all users to some common organization groups. This required some new privileges for the token, so the token has been switched out as well. It currently has: - admin read/write - organization read/write - repository read/write - user read/write Not sure if everything is needed, but I think it's a fair subset. admin read/write was already required to create users.
oysteikt added 1 commit 2024-08-26 21:44:14 +02:00
oysteikt requested review from felixalb 2024-08-26 21:44:22 +02:00
felixalb commented 2024-08-27 17:30:52 +02:00
Owner
Copy Link

Should the required token permissions be listed in the source code somewhere?

Should the required token permissions be listed in the source code somewhere?
felixalb approved these changes 2024-08-27 17:37:08 +02:00
felixalb left a comment
Owner
Copy Link

Some comments about separating "add" and "update", as they don't have to relate to each other at all, but it's not a blocker. This is after all Nitea, you can merge if you disagree.

Some comments about separating "add" and "update", as they don't have to relate to each other at all, but it's not a blocker. This is after all Nitea, you can merge if you disagree.
hosts/bekkalokk/services/gitea/import-users/gitea-import-users.py Outdated
@ -76,2 +97,2 @@
# Read the file, add each user
with open("/tmp/passwd-import", 'r') as f:
# Reads out a passwd-file line for line, and filters out
# real PVV users (as opposed to system users meant for daemons and such)
felixalb commented 2024-08-27 17:32:25 +02:00
Owner
Copy Link

Here, you are both explicitly allowing very long lines, and strictly wrapping the text. Maybe collapse this type of comment into single lines?

Here, you are both explicitly allowing very long lines, and strictly wrapping the text. Maybe collapse this type of comment into single lines?
oysteikt commented 2024-08-27 21:49:14 +02:00
Author
Owner
Copy Link

Eh, I'm not intentionally creating long lines, just not breaking the non-important ones (e.g. 100 char strings).

Eh, I'm not intentionally creating long lines, just not breaking the non-important ones (e.g. 100 char strings).
oysteikt marked this conversation as resolved
hosts/bekkalokk/services/gitea/import-users/gitea-import-users.py Outdated
@ -91,0 +133,4 @@
existing_users[username] = user
else:
user["visibility"] = existing_users[username]["visibility"]
felixalb commented 2024-08-27 17:33:37 +02:00
Owner
Copy Link

This "patch existing users" section should probably be it's own function, and be explained to show what it is / should be doing.

This "patch existing users" section should probably be it's own function, and be explained to show what it is / should be doing.
oysteikt commented 2024-08-27 21:47:52 +02:00
Author
Owner
Copy Link

It' not immediately clear to me at the moment how to best split this, when both depend on the same base user settings with name and username as input, and not just creating a bunch of overhead. I'll add a comment describing what it does though. Feel free to fix it up and ping me in another PR.

It' not immediately clear to me at the moment how to best split this, when both depend on the same base user settings with name and username as input, and not just creating a bunch of overhead. I'll add a comment describing what it does though. Feel free to fix it up and ping me in another PR.
oysteikt marked this conversation as resolved
hosts/bekkalokk/services/gitea/import-users/gitea-import-users.py Outdated
@ -91,0 +168,4 @@
for username, name in passwd_file_parser("/tmp/passwd-import"):
print(f"Processing {username}")
add_or_patch_gitea_user(username, name, existing_users)
felixalb commented 2024-08-27 17:35:10 +02:00
Owner
Copy Link

The splitting into "add" or "patch" should probably also be done here, rather than in one large function, as they are quite different.

The splitting into "add" or "patch" should probably also be done here, rather than in one large function, as they are quite different.
oysteikt marked this conversation as resolved
hosts/bekkalokk/services/gitea/import-users/gitea-import-users.py Outdated
@ -91,0 +169,4 @@
for username, name in passwd_file_parser("/tmp/passwd-import"):
print(f"Processing {username}")
add_or_patch_gitea_user(username, name, existing_users)
for org, team_name in COMMON_USER_TEAMS:
felixalb commented 2024-08-27 17:35:59 +02:00
Owner
Copy Link

This makes it explicitly impossible to leave either of those teams for good, is that intended? I suggest moving this into the "add new user" function, and not the "update existing user" section.

This makes it explicitly impossible to leave either of those teams for good, is that intended? I suggest moving this into the "add new user" function, and not the "update existing user" section.
oysteikt commented 2024-08-27 21:45:40 +02:00
Author
Owner
Copy Link

This is intended. All PVV members should be able to edit PVV projects. There are no downsides to being a member of the organization (at least when we fix the autowatch thing). We could alternatively provide the projects as an input to the script instead of hardcoding a list so it's more obvious from the nix code, but this will work for now. Maybe when there's more orgs to join

This is intended. All PVV members should be able to edit PVV projects. There are no downsides to being a member of the organization (at least when we fix the autowatch thing). We could alternatively provide the projects as an input to the script instead of hardcoding a list so it's more obvious from the nix code, but this will work for now. Maybe when there's more orgs to join
oysteikt marked this conversation as resolved
oysteikt commented 2024-08-27 21:52:41 +02:00
Author
Owner
Copy Link

Some comments about separating "add" and "update", as they don't have to relate to each other at all

"at all" is some strong wording. They didn't exist in the same function already if they weren't related to some degree. Adding user information and ensuring user information is correct, and those actions being mutually exclusive for every user in the loop is related enough that these were already put into a single function beforehand. But feel free to split it in another PR if you feel it would improve the code.

> Some comments about separating "add" and "update", as they don't have to relate to each other at all "at all" is some strong wording. They didn't exist in the same function already if they weren't related to some degree. Adding user information and ensuring user information is correct, and those actions being mutually exclusive for every user in the loop is related enough that these were already put into a single function beforehand. But feel free to split it in another PR if you feel it would improve the code.
oysteikt force-pushed gitea-import-users-add-to-members from 08fc48cf87 to 4cd2e6c4b0 2024-08-27 21:58:15 +02:00 Compare
oysteikt force-pushed gitea-import-users-add-to-members from 4cd2e6c4b0 to 4662e6b55d 2024-08-27 22:03:32 +02:00 Compare
oysteikt merged commit bd42412b94 into main 2024-08-27 22:07:30 +02:00
oysteikt deleted branch gitea-import-users-add-to-members 2024-08-27 22:07:30 +02:00
Sign in to join this conversation.
Reviewers
No reviewers
felixalb
Labels
Clear labels
  
art

The issue needs someone to be creative

  
backup

   
big

No, not bug, **big** - this is gonna take a while

  
blocked

This issue/PR depends on one or more other issues/PRs

  
bug

Something is not working, and it's not the shield hero

  
crash report

Report an oopsie

  
disputed

kranglefanter

  
documentation

Documentation changes required

  
duplicate

This issue or pull request already exists

  
enhancement

New feature

  
good first issue

Get your hands dirty with a new project here

  
logging

   
nixos

Requires changes to our nixos setup

  
question

More information is needed

  
salt

Requires changes to our salt setup

  
security

Skommel

  
servers n' hardware

Regards hardware, servers or VMs

  
wontfix

This won't be fixed

No Label
art
backup
big
blocked
bug
crash report
disputed
documentation
duplicate
enhancement
good first issue
logging
nixos
question
salt
security
servers n' hardware
wontfix
Milestone
Clear milestone
No items
No Milestone
Assignees
Clear assignees
No Assignees
2 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: Drift/pvv-nixos-config#70
No description provided.
Delete Branch "gitea-import-users-add-to-members"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?