coder / code-marketplace

Open source extension marketplace for VS Code.
GNU Affero General Public License v3.0
222 stars 23 forks source link

New Endpoint: /upload or /publish extension #18

Open podedra92 opened 1 year ago

podedra92 commented 1 year ago

We currently have the Add cli command, would be beneficial to expose this via API. This would prevent the need to exec into the pods running in the cluster to add a new extension.

alexp73 commented 9 months ago

As a part of our internal project we need to implement new /add and /remove endpoints. Please, see POC below (you can copy the yaml to editor.swagger.io). The idea is to repeat CLI interface for add and remove commands with one exception: the user can provide "extension" field (to upload extensions) or "extensionUrl" (to add extension using URL). Please, check and comment, any suggestions would be really useful. Thanks.

openapi: 3.0.3
info:
  title: Coder Marketplace New APIs
  description: |-
    This is a proposal for new "Add extension" and "Remove extension" API endpoints
  version: 0.0.1
servers:
  - url: https://example.com/api
paths:
  /add:
    post:
      tags:
        - extensions
      summary: Add extension(s)
      description: Add new extension(s) to the marketplace, extension or extension-url parameter should be provided
      operationId: addExtension
      requestBody:
        content:
          multipart/form-data:
            schema:
              type: object
              properties:
                extensions-dir:
                  type: string
                artifactory:
                  type: string
                repo:
                  type: string
                extension:
                  type: string
                  format: binary
                extensionUrl:
                  type: string
        required: true
      responses:
        '201':
          description: Extension added
        '400':
          description: Unable to add extension
          content:
            application/json:
              schema:
                $ref: '#/components/schemas/AddApiResponse'
  /remove/{extensionId}:
    delete:
      tags:
        - extensions
      summary: Remove extension
      description: Remove an extension, one version or all versions, if "all" keyword provided
      operationId: removeExtension
      parameters:
        - name: extensionId
          in: path
          description: Extension ID or "all" keyword
          required: true
          schema:
            type: string
      responses:
        '400':
          description: Unable to remove extension
          content:
            application/json:
              schema:
                $ref: '#/components/schemas/RemoveApiResponse'
components:
  schemas:
    RemoveApiResponse:
      type: object
      properties:
        message:
          type: string
          example: Unable to remove extension
    AddApiResponse:
      type: object
      properties:
        message:
          type: string
          example: Unable to add extension
code-asher commented 9 months ago

This looks really good! Some thoughts:

  1. Can we omit extensions-dir, artifactory, and repo for /add? The marketplace would already know all these values. If we do need them, then I imagine the remove endpoint also needs them.
  2. I think I would expect the /remove endpoint to take all as a query parameter to mimic how the cli does it, so something like /remove/{id}?all.
  3. While the /add, /remove endpoints do make sense and I see how they mirror the cli, I think it would be more REST-like to use /extensions and /extensions/{id}.
  4. Authentication is something we will have to figure out if this gets added. Maybe it can even just be basic http auth. Or, we could make the new endpoints opt-in and users have to secure the endpoints themselves.
alexp73 commented 9 months ago

Thank you for your response.

  1. You are right. Just confirmed we can re-use them.
  2. Yes, I have mentioned "all" in the description and going to support it.
  3. Yes, agree with that. I will use POST /extensions and DELETE /extensions{extensionId} or DELETE /extensions{extensionId@all} (@ because is used for versions now: https://github.com/coder/code-marketplace/pull/24) as you suggested.
  4. Can we do authentication as a next step or do you think it should be the part of the same PR?
code-asher commented 9 months ago

Yes, I have mentioned "all" in the description and going to support it. /extensions{extensionId@all}

Ohhhh I see, yes this makes sense, I misread it as /extensions/all. I like that, maybe we should do that in the cli as well, right now the cli expects remove ext --all rather than remove ext@all, but we can think about changing the cli later.

Can we do authentication as a next step or do you think it should be the part of the same PR?

It does not need to be part of the same PR, but I would say at the very least we disable these endpoints by default, just to make sure we do not accidentally release with the endpoints exposed.

alexp73 commented 9 months ago

Ohhhh I see, yes this makes sense, I misread it as /extensions/all. I like that, maybe we should do that in the cli as well, right now the cli expects remove ext --all rather than remove ext@all, but we can think about changing the cli later.

Agreed, let's think about cli changes later.

Can we do authentication as a next step or do you think it should be the part of the same PR?

It does not need to be part of the same PR, but I would say at the very least we disable these endpoints by default, just to make sure we do not accidentally release with the endpoints exposed.

Noted, I will disable the endpoints by default. I think I will add some environment variables support: ENABLE_PUBLISHING_ENDPOINTS, MAX_UPLOAD_FILE_SIZE, MAX_FILES and UPLOADS_FOLDER.

code-asher commented 9 months ago

I think I will add some environment variables support: ENABLE_PUBLISHING_ENDPOINTS, MAX_UPLOAD_FILE_SIZE, MAX_FILES and UPLOADS_FOLDER.

Works for me! Should we prefix these with MARKETPLACE or CODE_MARKETPLACE (a bit long but being unique seems like a good idea)?

alexp73 commented 9 months ago

I think I will add some environment variables support: ENABLE_PUBLISHING_ENDPOINTS, MAX_UPLOAD_FILE_SIZE, MAX_FILES and UPLOADS_FOLDER.

Works for me! Should we prefix these with MARKETPLACE or CODER_MARKETPLACE (a bit long but being unique seems like a good idea)?

Sure, I will add the MARKETPLACE prefix.

alexp73 commented 8 months ago

Quick update on this one. Functionally, everything is done and UT added but the changes are still inside our organisation. We are going to setup the process to contribute the changes back, using our organisation's account. It will take some time (2-3 weeks, I hope not more). In the mean time, I'm starting to work on protecting these endpoints by authentication.