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

New server #130

Closed
wants to merge 1 commit into from
Closed

New server #130

wants to merge 1 commit into from

Conversation

SteBaum
Copy link

@SteBaum SteBaum commented Nov 28, 2023

Discussion

This PR is a server started from scratch although it has been inspired by the previous one.

  • Switched to python version 3.9 and newer depencies in pyproject.toml.
  • The endpoints have been changed.
  • The schemas are not identical.

Agreements

@SteBaum SteBaum force-pushed the new-server branch 2 times, most recently from 12cbcb4 to 6594881 Compare November 28, 2023 16:08
@PaulFarault PaulFarault self-requested a review November 29, 2023 15:29
@PaulFarault
Copy link
Contributor

PaulFarault commented Nov 29, 2023

Please add tdp-lib as a dependency instead of a requirement if needed.

Copy link
Contributor

@PaulFarault PaulFarault left a comment

Choose a reason for hiding this comment

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

We may want to:

  • Add pagination on endpoints that return a list.
  • Use dedicated endpoints to allow POST on variables (such as /services/[service-id]/variables and /services/[service-id]/components/[component-id]/variables or even moving it under the /variables endpoint).
  • Remove POST /variables/{import_file}, this doesn't make sense.
  • Change /variables/validation from a POST to a GET method. Also, it could be moved to only /validate-variables or /validate.
  • Drop the usage of nouns to describe actions endpoints. We are not exactly doing REST here.
  • Remove the /status endpoint if its informations can be returned in the "services" and "components" endpoints.
  • Fix deployments POST routes (idk what "Post Dag", "Post Operations", "Post Resumumption" and "Post Reconfiguration" refer to). We may also want to move the deploy action out:
    • GET /deployments: list past deployments.
    • GET /deployments/{deployment-id}: display a deployment.
    • GET /deployments/{deployment-id}/operations/{operation-order}/logs: display complete logs for an operation.
    • POST /deploy: launch the planned deployment.
    • POST /plan/[from-dag|from-operations|import|reconfigure|resume]: create a new deployment plan.
  • Add the following missing endpoints:
    • GET the variables schemas.
    • POST a custom deployment plan (used for plan edition and creation from scratch).
    • GET status history for a single component.
  • Use 404 instead of 400 when a given id doesn't exist.

I may miss some things but that would be a good start.

Copy link
Contributor

@PaulFarault PaulFarault left a comment

Choose a reason for hiding this comment

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

  • Why did you dissociate /services from /services/status? Same for specific services and components. You can send status infos on the main endpoints. For example, GET /services/{service-id}:

    {
      ...pagination infos
      "items": [
        {
          "id": "hdfs",
          "name": "HDFS",
          "status": "RUNNING",
          "configured_version": "ff4627859010bbd6f43808b51121972c0345bbc0",
          "running_version": "ff4627859010bbd6f43808b51121972c0345bbc0",
          "variables_url": "https://...",
          "schema_url": "https://..."
        },
        ...
      ]
    }
  • It seems that you've also got some trouble with pagination. Pagination options should NOT appear on POST endpoints.

  • /plan endpoint miss options.

  • We may want to use options instead of specific endpoint to filter operations (/operations/dag and /operations/others).

  • GET /variables is not useful. Variables are specified for a component or a service. That doesn't make sense to fetch all variables at once from the same endpoint.

  • Same for GET /variables/schemas, they are directly linked to a service, we could fetch them with GET /service/{service-ud}/schema.

  • GET /deployment/{deployment-id} response could be more structured. You should dissociate values that are common to all deployments from the "deployment options" which depends on the deployment type.

  • I'm not sure about the use of a 422 on GET endpoints (GET /services/{service-id} for example).

  • You can add a GET method on /services/{service-id}/variables (same for components).

I saw that you didn't fixed all of my first batch of comments. Don't hesitate to comment if you want / need to discuss some of them.

README.md Outdated Show resolved Hide resolved
@SteBaum
Copy link
Author

SteBaum commented Dec 6, 2023

  • Created a new endpoint for status which is equivalent to the tdp-lib tdp status show command.
  • The add_pagination library automatically joins the 422 error each time we implement it in an endpoint.

@SteBaum
Copy link
Author

SteBaum commented Dec 6, 2023

Removed the methods put and patch in the endpoints services and components since their goal was to update the variables of the service/component and we have now a service/variables and component/variables endpoint. However I would like to have your opinion on that.

@PaulFarault
Copy link
Contributor

PaulFarault commented Dec 11, 2023

  • add back the status edition endpoints (PUT /services/{service-id}, same on components)
  • most of the body schema that you provide should be transform to schema for query parameters
  • GET /services/{service-id} shouldn't return variables, only variables-url. Add a GET /services/{service-id}/variables for this purpose (same for component).
  • add a GET /services/{service-id}/schema
  • POST on variables should be converted to a PUT
  • GET /deployments/{deployment-id} shouldn't return operation text logs

@PaulFarault
Copy link
Contributor

PaulFarault commented Dec 15, 2023

  • /api/v1/services/{service_id}/variables should return a dict with variables. No need for pagination.
  • /services/{service_id}/schema is called "Get Service Variables".
  • /services/{service_id}/components/{component_id}/status-history should return a list.
  • What is the use of /services/{service_id}/components/{component_id}/stales as service and components can be edited through their PUT endpoints. You should remove this one and add to_config, to_restart, running_version and configured_version as query parameters of PUT /api/v1/services/{service_id} and /api/v1/services/{service_id}/components/{component_id}. Also id, variables_url and schema_url shouldn't be editable.
  • /api/v1/deployments/{deployement_id} should return a dict.
  • Please format the "Discussions" section of your README to make it easier to comment:
    • Split general REST discussions from our endpoints choices. Provide external sources for general REST discussions and eventually some Ambary / tdp server v0.1 refs for the second part.

    • Format each discussion as:

      - bias
        discussion

@PaulFarault
Copy link
Contributor

PaulFarault commented Dec 18, 2023

  • /services/{service_id}/schema misses a response sample (null for now).

  • We may want to provide navigation url for "items" lists. For example in /api/v1/services.items, instead of returning:

      {
      "items": [
              "hdfs"
          ],
      "total": 1,
      ...
      }

    We could have:

        {
      "items": [
          {
              "id": "hdfs",
              "service_url": "http://.../api/v1/services/hdfs",
          }
          ],
      "total": 1,
      ...
      }

    Same for /api/v1/deployments/{deployement_id}.operations).

  • GET /api/v1/services/{service_id} misses to_config and to_restart, also status is not needed here. It'll be handled by tdp_observability.

  • What is host on GET /api/v1/services/{service_id}/components/{component_id}?

  • host should be a parameter on GET /api/v1/services/{service_id}/components/{component_id}/status-history.

  • POST /api/v1/deploy why is vars a query parameter?

  • I'm not sure why {Operationtype} appears in the path on GET api/v1/operations/{Operationtype} as it is a query parameter.

  • POST /api/v1/plan/dag I'm not sure to understand the use of the options query parameter

  • Please check the response schema of the POST /api/v1/plan/... endpoints (will noop, depends_on, ... really needed? Also, it lacks deployment information as it's id).

  • Would be nice to have GET / return a list of URL to all available GET endpoints.

I wonder if:

  • we should merge status, services, components and validate groups to configurations.
  • GET /deployments/status should be moved to GET /deploy as it is the endpoint that trigger the deploy action.

@SteBaum
Copy link
Author

SteBaum commented Dec 19, 2023

The response_schema for POST /api/v1/plan/... endpoints are almost identical to the result given by tdp-lib

@PaulFarault
Copy link
Contributor

PaulFarault commented Dec 20, 2023

  • I'm not sure if service_id should be returned by GET /api/v1/services/{service_id}/components/{component_id}.
  • Why did you add /get-endpoints ?
  • Response sample are wrong for plan endpoints (deployment is not executed for them, no time should be returned). Also, you can uses the same kind of object schema than for the lib (including an options object).

Also, we should list components available for a service somewhere.

@SteBaum
Copy link
Author

SteBaum commented Dec 22, 2023

The /get-endpoints is an endpoint which justs lists all available endpoints with the GET method

@SteBaum
Copy link
Author

SteBaum commented Dec 22, 2023

I have alos added a test framework which for now only has 3 simple tests, but we will work on that once we start implementing the endpoint functions

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.

None yet

3 participants