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

Change osggid assigner script to do project group setup (SOFTWARE-5619) #21

Conversation

williamnswanson
Copy link
Collaborator

Add UNIX Cluster Group creation and LDAP Provisioning to the osggid assigner script.
Also renamed the script to "project_group_setup" to fit it's new role.

@williamnswanson williamnswanson self-assigned this Dec 5, 2023
Copy link
Member

@brianhlin brianhlin left a comment

Choose a reason for hiding this comment

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

In addition to the comments in-line, I've got a few general thoughts:

  1. There's a lot of duplication with group_fixup.py that should be refactored out into a separate library
  2. Lots of your logic is crammed into main(). You should split it out into small functions and call them
  3. More comments! Some parts of the script are a little unclear and reviewing would be a bit easier with some explanations on the side.
  4. It's better to ask for forgiveness than for permission! There are a few spots in your script where your if statements should just be try/except. It'll help with some of the deep nesting and thus readability of the script

project_group_setup.py Outdated Show resolved Hide resolved
project_group_setup.py Show resolved Hide resolved
project_group_setup.py Outdated Show resolved Hide resolved
project_group_setup.py Outdated Show resolved Hide resolved
project_group_setup.py Outdated Show resolved Hide resolved
project_group_setup.py Outdated Show resolved Hide resolved
project_group_setup.py Outdated Show resolved Hide resolved
project_group_setup.py Outdated Show resolved Hide resolved
@williamnswanson williamnswanson force-pushed the SOFTWARE-5619.assign-new-project-gids branch 2 times, most recently from 998ffc3 to efdd969 Compare December 8, 2023 20:56
@williamnswanson williamnswanson force-pushed the SOFTWARE-5619.assign-new-project-gids branch from efdd969 to 458af37 Compare December 8, 2023 20:57
Also, move get_projects_to_setup() functionality into several different methods for clarity.
@williamnswanson williamnswanson force-pushed the SOFTWARE-5619.assign-new-project-gids branch from 40b6445 to 172acae Compare December 20, 2023 17:44
Created a file to hold methods that are, or will likely be, used by multiple scripts in the osg-comanage-rpoject-usermap repo. Refactored existing scripts to use the new library.
Projects that needed an OSGGID identifier weren't being provisioned if a UNIX cluster group already existed. Added identifier updating after adding missing identifiers and moved adding identifiers to before checking what projects need UNIX cluster groups and provisioning.
@williamnswanson williamnswanson force-pushed the SOFTWARE-5619.assign-new-project-gids branch from 172acae to 6978619 Compare December 20, 2023 17:49
Copy link
Member

@brianhlin brianhlin left a comment

Choose a reason for hiding this comment

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

I'd rename comanage_scripts_utils.py -> comanage_utils.py. The scripts is a bit superfluous.

But otherwise just a few nitpicks! Preapproving

create_project.py Show resolved Hide resolved
project_group_setup.py Outdated Show resolved Hide resolved
comanage_scripts_utils.py Outdated Show resolved Hide resolved
project_group_setup.py Outdated Show resolved Hide resolved
project_group_setup.py Outdated Show resolved Hide resolved
project_group_setup.py Outdated Show resolved Hide resolved
project_group_setup.py Show resolved Hide resolved
project_group_setup.py Show resolved Hide resolved
project_group_setup.py Outdated Show resolved Hide resolved
project_group_setup.py Outdated Show resolved Hide resolved
@williamnswanson williamnswanson merged commit a99f06e into opensciencegrid:master Jan 18, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants