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

MultiRegistry Beta API #944

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

Conversation

nayanjd-do
Copy link

@nayanjd-do nayanjd-do commented Nov 13, 2024

This PR adds the new endpoints for MultiRegistry Beta in Container Registry Product.

Endpoints added:

  • GET /v2/registries/
  • GET /v2/registries/{registry_name}
  • POST /v2/registries
  • DELETE /v2/registries/{registry_name}
  • GET /v2/registries/{registry_name}/docker-credentials

JIRA: DOCR-1003
Channel: #docr-eng

@nayanjd-do nayanjd-do changed the title MultiRegistry Beta API WIP: MultiRegistry Beta API Nov 13, 2024
@nayanjd-do nayanjd-do changed the title WIP: MultiRegistry Beta API MultiRegistry Beta API Nov 15, 2024
Copy link
Member

@andrewsomething andrewsomething left a comment

Choose a reason for hiding this comment

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

Overall this looks great. A few suggestions and questions inline.

curl -X GET \
-H "Content-Type: application/json" \
-H "Authorization: Bearer $DIGITALOCEAN_TOKEN" \
"https://api.digitalocean.com/v2/registry"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"https://api.digitalocean.com/v2/registry"
"https://api.digitalocean.com/v2/registries"

@@ -0,0 +1,51 @@
operationId: multiregistry_create
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
operationId: multiregistry_create
operationId: registries_create

The operation ID is used by generated clients to name methods. So that we are consistent with godo and other resources, we probably want to use registries here in place of multiregistry.

@@ -0,0 +1,51 @@
operationId: multiregistry_create

summary: Create Container Registry By Name (Beta)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
summary: Create Container Registry By Name (Beta)
summary: [Beta] Create Container Registry By Name

Let's put the beta tag up front for clarity.

Comment on lines +1 to +3
operationId: multiregistry_delete

summary: Delete Container Registry By Name (Beta)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
operationId: multiregistry_delete
summary: Delete Container Registry By Name (Beta)
operationId: registries_delete
summary: [Beta] Delete Container Registry By Name

Comment on lines +1 to +3
operationId: multiregistry_get

summary: Get Container Registry By Name (Beta)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
operationId: multiregistry_get
summary: Get Container Registry By Name (Beta)
operationId: registries_get
summary: [Beta] Get a Container Registry By Name

Comment on lines +1 to +3
operationId: multiregistry_get_all

summary: Get All Container Registries (Beta)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
operationId: multiregistry_get_all
summary: Get All Container Registries (Beta)
operationId: registries_list
summary: [Beta] List All Container Registries

Comment on lines +1 to +3
operationId: multiregistry_get_dockerCredentials

summary: Get Docker Credentials By Registry Name (Beta)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
operationId: multiregistry_get_dockerCredentials
summary: Get Docker Credentials By Registry Name (Beta)
operationId: registries_get_dockerCredentials
summary: [Beta] Get Docker Credentials By Registry Name

"region": "fra1"
}

resp = client.registry.create(body=req)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
resp = client.registry.create(body=req)
resp = client.registries.create(body=req)

As mentioned, these are generated from the operation ID. So at the moment, that would actually be client.multiregistry.create, but I think we really want client.registries.create.

curl -X GET \
-H "Content-Type: application/json" \
-H "Authorization: Bearer $DIGITALOCEAN_TOKEN" \
"https://api.digitalocean.com/v2/registries"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"https://api.digitalocean.com/v2/registries"
"https://api.digitalocean.com/v2/registries/example"

@@ -0,0 +1,39 @@
type: object
Copy link
Member

Choose a reason for hiding this comment

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

This is nearly identical to the existing registry model. It's just missing the subscription piece. I wonder if we could do this in a way that doesn't require duplication?

Maybe something like rename this to models/registry_base.yml and update the models/registry.yml to be:

type: object

allOf:
  - $ref: 'registry_base.yml'
  - type: object
    properties:
      subscription:
        allOf:
          - readOnly: true
          - $ref: 'subscription.yml'

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.

3 participants