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(BA-483): Revamp ContainerRegistryNode API #3424

Draft
wants to merge 16 commits into
base: topic/11-11-feat_implement_associatecontainerregistrywithgroup_disassociatecontainerregistrywithgroup_
Choose a base branch
from

Conversation

jopemachine
Copy link
Member

@jopemachine jopemachine commented Jan 10, 2025

Resolves #3418 (BA-483).

Outlines

Caution

This PR include breaking change.

This PR performs the following tasks:

GQL example

Refer to the schema.graphql file to see how the input types for each GQL query and mutation have changed.
In this section, I will explain each GQL usage with examples.

create_container_registry_node

mutation {
  create_container_registry_node (props: { type: "harbor2", registry_name: "cr.backend.ai", url: "https://cr.backend.ai", project: "stable", allowed_groups: { add: ["2de2b969-1d04-48a6-af16-0bc8adb3c831"] } }) { 
    container_registry {
      id
      row_id
      name
    }
  }

modify_container_registry_node

mutation {
  modify_container_registry_node (id: "5c7090ca-0422-4ed6-86c1-7bba2a5de153", props: { registry_name: "cr.backend.ai2", url: "https://abcdef", project: "", allowed_groups: { remove: ["2de2b969-1d04-48a6-af16-0bc8adb3c831"] } }) {
    container_registry {
      id
      row_id
      name
    }
  }
}

delete_container_registry_node

mutation {
  delete_container_registry_node (id: "5c7090ca-0422-4ed6-86c1-7bba2a5de153") {
    container_registry {
      id
      row_id
      name
    }
  }
}

allowed_groups

query {
  container_registry_node(id: "<encoded registry_id>") {
    allowed_groups (limit: 5, offset:0) {
      name
      created_at
    }
  }
}

Checklist: (if applicable)

  • Milestone metadata specifying the target backport version
  • Mention to the original issue
  • API server-client counterparts (e.g., manager API -> client SDK)
  • Test case(s) to:
    • Demonstrate the difference of before/after
    • Demonstrate the flow of abstract/conceptual models with a concrete implementation
  • Documentation
    • Contents in the docs directory
    • docstrings in public interfaces and type annotations

📚 Documentation preview 📚: https://sorna--3424.org.readthedocs.build/en/3424/


📚 Documentation preview 📚: https://sorna-ko--3424.org.readthedocs.build/ko/3424/

@jopemachine jopemachine force-pushed the topic/01-10-refactor_revamp_containerregistrynode_api branch from adc707a to 265cecf Compare January 10, 2025 05:39
@jopemachine jopemachine changed the base branch from topic/11-06-feat_implement_per-project_images_api_based_on_rbac to topic/11-11-feat_implement_associatecontainerregistrywithgroup_disassociatecontainerregistrywithgroup_ January 10, 2025 05:39
@jopemachine jopemachine changed the title refactor: Revamp ContainerRegistryNode API refactor(BA-483): Revamp ContainerRegistryNode API Jan 10, 2025
@jopemachine jopemachine added this to the 25Q1 milestone Jan 10, 2025
@jopemachine jopemachine force-pushed the topic/01-10-refactor_revamp_containerregistrynode_api branch from 265cecf to bb4b42a Compare January 10, 2025 06:04
@jopemachine jopemachine force-pushed the topic/11-11-feat_implement_associatecontainerregistrywithgroup_disassociatecontainerregistrywithgroup_ branch from 23a194c to 978b571 Compare January 10, 2025 06:11
@jopemachine jopemachine force-pushed the topic/01-10-refactor_revamp_containerregistrynode_api branch 2 times, most recently from 4113540 to 0c45704 Compare January 13, 2025 00:38
@github-actions github-actions bot added the comp:common Related to Common component label Jan 13, 2025
@jopemachine jopemachine marked this pull request as ready for review January 14, 2025 06:10
@jopemachine jopemachine force-pushed the topic/01-10-refactor_revamp_containerregistrynode_api branch from 17c3313 to a76fd01 Compare January 14, 2025 06:18
@jopemachine jopemachine marked this pull request as draft January 16, 2025 07:34
@jopemachine jopemachine force-pushed the topic/01-10-refactor_revamp_containerregistrynode_api branch from 0d29589 to cd53aa4 Compare January 21, 2025 02:25
@jopemachine jopemachine force-pushed the topic/01-10-refactor_revamp_containerregistrynode_api branch from cd53aa4 to f5d681b Compare January 22, 2025 05:32
Comment on lines +1191 to +1199
if target_scope is not None:
permission_ctx = await builder.build(ctx, target_scope, requested_permission)
elif container_registry_scope is not None:
permission_ctx = await builder.build_ctx_in_container_registry_scope(
ctx, container_registry_scope
)
else:
raise ValueError("either target_scope or container_registry_scope must be provided")

Copy link
Member

Choose a reason for hiding this comment

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

In our RBAC system, target scope should not be null. You can use SystemScope if you need to query in a system wide scope.

before: str | None = None,
last: int | None = None,
scope: Optional[ScopeType] = None,
container_registry_scope: Optional[ContainerRegistryScope] = None,
Copy link
Member

Choose a reason for hiding this comment

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

Just an idea. Since we are likely to have more orthogonal scopes mapped to project scopes, we could define a parameter type that contains container_registry_scope and use it here.
e.g.

@dataclass # Can replace this with pydantic, TypedDict whatever you want!
class ProjectAdditionalScope:
    container_registry_scope: Optional[ContainerRegistryScope] = None

async def get_connection(
        cls,
        info: graphene.ResolveInfo,
        scope: Optional[ScopeType] = None,
        extra_scopes: ProjectAdditionalScope,

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:docs Documentations comp:client Related to Client component comp:common Related to Common component comp:manager Related to Manager component size:XL 500~ LoC
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants