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

[WIP] Add test node #377

Closed
wants to merge 3 commits into from
Closed

Conversation

Bzil
Copy link
Contributor

@Bzil Bzil commented Jul 4, 2024

TODO

Description

Describe what this change achieves.

Issues Resolved

List any issues this PR will resolve, e.g. Closes [...].

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Bzil added 2 commits July 4, 2024 14:19
@@ -268,6 +268,21 @@ paths:
responses:
'200':
$ref: '#/components/responses/nodes.info@200'
/_nodes/{metric}/stats:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

This path already exists in the spec, it is /_nodes/{node_id}/stats and is part of the nodes.stats operation not nodes.info

Copy link
Contributor

github-actions bot commented Jul 4, 2024

Changes Analysis

Commit SHA: a37033c
Comparing To SHA: cbc0b08

API Changes

Summary


├─┬Paths
│ └──[➕] path (2929:3)
└─┬Components
  ├─┬nodes.info:Metric
  │ ├──[➕] enum (45315:11)
  │ └──[➕] enum (45314:11)
  └─┬nodes.info:ResponseBase
    └─┬ALLOF
      ├──[➖] required (45971:15)❌ 
      └─┬nodes
        ├──[➖] additionalProperties (45966:17)❌ 
        ├──[➖] type (45964:21)❌ 
        ├──[➕] oneOf (45317:7)
        └──[➕] oneOf (45989:19)

Document Element Total Changes Breaking Changes
paths 1 0
components 7 6
  • BREAKING Changes: 6 out of 8
  • Removals: 3
  • Additions: 5
  • Breaking Removals: 3

Report

The full API changes report is available at: https://github.com/opensearch-project/opensearch-api-specification/actions/runs/9808029450/artifacts/1671078058

API Coverage

Before After Δ
Covered (%) 483 (47.31 %) 483 (47.31 %) 0 (0 %)
Uncovered (%) 538 (52.69 %) 538 (52.69 %) 0 (0 %)
Unknown 24 24 0

@Bzil Bzil force-pushed the add-test-node branch from 2152886 to a37033c Compare July 5, 2024 12:06
method: GET
parameters:
metric:
- data:true
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dblock do you have any idea how to implement a fix for this test ?

Copy link
Member

Choose a reason for hiding this comment

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

This error?

FAILED  Nodes _cluster_manager metric with a json response. 
            FAILED  PARAMETERS 
                FAILED  metric (data/0 must be equal to one of the allowed values)

The data:true value doesn't seem like a valid metric.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's seems weird yes, but it's define in documentation

Copy link
Member

Choose a reason for hiding this comment

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

Is it read like a JSON value? Quote it, "data:true"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if I do that, that implies that I have to put it in the enum and therefore do I have to do the same for data:false and the other filters?

Copy link
Member

Choose a reason for hiding this comment

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

I suppose that yes? There's really no such thing as booleans in URL paths AFAIK, this is a very creative use of the API.

Copy link
Collaborator

Choose a reason for hiding this comment

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

These should be using /_nodes/{node_id}/stats

@@ -268,6 +268,21 @@ paths:
responses:
'200':
$ref: '#/components/responses/nodes.info@200'
/_nodes/{metric}/stats:
Copy link
Collaborator

Choose a reason for hiding this comment

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

This path already exists in the spec, it is /_nodes/{node_id}/stats and is part of the nodes.stats operation not nodes.info

Comment on lines +24 to +25
- _all
- _cluster_manager
Copy link
Collaborator

Choose a reason for hiding this comment

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

These are node filters not metric names

Comment on lines 32 to 42
nodes:
type: object
additionalProperties:
$ref: '#/components/schemas/NodeInfo'
oneOf:
- type: object
$ref: '#/components/schemas/NodeInfo'
- type: object
additionalProperties: {}
_nodes:
$ref: 'nodes._common.yaml#/components/schemas/NodesResponseBase'
required:
- cluster_name
- nodes
- _nodes
Copy link
Collaborator

Choose a reason for hiding this comment

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

These changes are likely due to the duplicate path and having the wrong schemas, please revert them and test again with other changes made.

@@ -7,3 +7,53 @@ chapters:
method: GET
Copy link
Collaborator

Choose a reason for hiding this comment

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

This file should be moved under tests/nodes/ and split into info.yaml and stats.yaml

method: GET
parameters:
metric:
- data:true
Copy link
Collaborator

Choose a reason for hiding this comment

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

These should be using /_nodes/{node_id}/stats

response:
status: 200
- synopsis: Nodes _all metric with a json response.
path: /_nodes/{metric}
Copy link
Collaborator

Choose a reason for hiding this comment

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

This one is technically /_nodes/{node_id}

@dblock
Copy link
Member

dblock commented Jul 31, 2024

Last tests from this PR made it into #455.

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