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

refactor: Add users_to_add and users_to_remove fields to replace user update mode #1705

Open
wants to merge 33 commits into
base: main
Choose a base branch
from

Conversation

fregataa
Copy link
Contributor

@fregataa fregataa commented Nov 8, 2023

partial-fix #1697

  1. ModifyGroup mutation has been used to add users to group and delete users from group.
    Let's refactor the mutation.

  2. Revert some description/deprecation_reason string generator functions for good readability

Test query

mutation ModifyGroup {
    modify_group(
        gid: "GROUP_ID",
        props: {
            add_users: [],
            remove_users: ["USER_ID"],
        }
    ) {
        ok
        msg
        group {
            name
        }
    }
}

Checklist: (if applicable)

  • Milestone metadata specifying the target backport version

@fregataa fregataa added this to the 24.03 milestone Nov 8, 2023
@fregataa fregataa self-assigned this Nov 8, 2023
@github-actions github-actions bot added comp:manager Related to Manager component type:refactor Refactoring current implementation. urgency:blocker IT SHOULD BE RESOLVED BEFORE NEXT RELEASE! size:L 100~500 LoC labels Nov 8, 2023
src/ai/backend/manager/models/group.py Outdated Show resolved Hide resolved
@fregataa fregataa changed the title refactor: Add user add/delete to group mutation refactor: Add added_users and removed_users fields to replace user update mode Nov 24, 2023
Copy link
Member

@achimnol achimnol left a comment

Choose a reason for hiding this comment

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

Since this is an active API operation, we should use the active voice for the field names.

Let's rename:

  • added_usersadd_users
  • removed_usersremove_users

@fregataa fregataa changed the title refactor: Add added_users and removed_users fields to replace user update mode refactor: Add add_users and remove_users fields to replace user update mode Jan 3, 2024
@fregataa fregataa requested a review from achimnol January 3, 2024 08:02
@fregataa fregataa added this to the 24.09 milestone Apr 23, 2024
@fregataa fregataa removed the urgency:blocker IT SHOULD BE RESOLVED BEFORE NEXT RELEASE! label Apr 23, 2024
@github-actions github-actions bot added the urgency:blocker IT SHOULD BE RESOLVED BEFORE NEXT RELEASE! label Apr 23, 2024
@github-actions github-actions bot modified the milestones: 24.09, 23.09 Apr 23, 2024
@fregataa fregataa removed the urgency:blocker IT SHOULD BE RESOLVED BEFORE NEXT RELEASE! label Apr 25, 2024
@github-actions github-actions bot added the urgency:blocker IT SHOULD BE RESOLVED BEFORE NEXT RELEASE! label Apr 25, 2024
@fregataa fregataa modified the milestones: 23.09, 24.09 Apr 30, 2024
@fregataa fregataa removed the urgency:blocker IT SHOULD BE RESOLVED BEFORE NEXT RELEASE! label Apr 30, 2024
src/ai/backend/manager/models/group.py Outdated Show resolved Hide resolved
src/ai/backend/manager/models/group.py Outdated Show resolved Hide resolved
@fregataa fregataa requested a review from achimnol May 8, 2024 09:28
Copy link
Member

@achimnol achimnol left a comment

Choose a reason for hiding this comment

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

Other parts looks good. Just a minor update please.

src/ai/backend/manager/models/group.py Outdated Show resolved Hide resolved
@fregataa fregataa requested a review from achimnol May 9, 2024 09:08
changes/1705.fix.md Outdated Show resolved Hide resolved
@agatha197 agatha197 changed the title refactor: Add add_users and remove_users fields to replace user update mode refactor: Add users_to_add and users_to_remove fields to replace user update mode May 10, 2024
Copy link
Contributor

@agatha197 agatha197 left a comment

Choose a reason for hiding this comment

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

LGTM

agatha197

This comment was marked as duplicate.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
comp:manager Related to Manager component size:M 30~100 LoC type:refactor Refactoring current implementation. urgency:4 As soon as feasible, implementation is essential.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Deprecate ModifyGroup.{user_uuids,user_update_mode} and add AddUsersToGroup, RemoveUsersFromGroup mutations
4 participants