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

Expose capability checks for YAML REST tests #108425

Merged
merged 8 commits into from
May 10, 2024

Conversation

mosche
Copy link
Contributor

@mosche mosche commented May 8, 2024

This PR adds support to check capabilities both in the skip and requires section on YAML REST tests.

See here for an example:

"Capabilities API":

  - requires:
      capabilities:
        - method: GET
          path: /_capabilities
          parameters: [method, path, parameters, capabilities]
          capabilities: []
      reason: "capabilities api requires itself to be supported"

  - do:
      capabilities:
        method: GET
        path: /_capabilities
        parameters: method,path,parameters,capabilities
        error_trace: false

  - match: { supported: true }

@mosche mosche added >test Issues or PRs that are addressing/adding tests :Core/Infra/Core Core issues without another label Team:Core/Infra Meta label for core/infra team labels May 8, 2024
@mosche mosche requested review from thecoop and a team May 8, 2024 17:07
@mosche mosche marked this pull request as ready for review May 8, 2024 17:07
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra (Team:Core/Infra)

@mosche
Copy link
Contributor Author

mosche commented May 8, 2024

:rest-api-spec:validateRestSpec returns an error for capabilities.json, apparently I skipped over some required fields.

@thecoop
Copy link
Member

thecoop commented May 9, 2024

I've added an unknown response to a capabilities query, indicating the cluster can't determine if something is supported or not - eg its a mixed-cluster and one of the nodes doesn't support _capabilities, or there's an error communicating with a node.

That does mean that the response can be either a string or a boolean - not great. The alternatives are:

  • Return an HTTP error 503 if we get a failure from a node
  • Use a tri-state value true false null
  • Add an extra field indicating if the result can be computed properly "available": true, "supported": false. This might be a bit confusing.

We also need to consider what clients will do if a node temporarily goes unavailable, resulting in all capability queries returning null/unknown as it can't get a response off a node to check its capabilities.

@thecoop
Copy link
Member

thecoop commented May 9, 2024

Decision - return null when some nodes are unresponsive. We should also return false if any nodes do not support _capabilities at all

@mosche
Copy link
Contributor Author

mosche commented May 10, 2024

Thanks @thecoop , the changes are looking good to me 💯
How do you want to proceed?

@thecoop
Copy link
Member

thecoop commented May 10, 2024

I've changed this to use null for unknown, which gets all these tests passing. Making it return false when some nodes don't support capabilities (with tests) can be a separate PR.

Copy link
Member

@thecoop thecoop left a comment

Choose a reason for hiding this comment

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

I'm happy with this, @mosche merge if you are

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Core/Infra/Core Core issues without another label Team:Core/Infra Meta label for core/infra team >test Issues or PRs that are addressing/adding tests v8.15.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants