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

Document usage of Capabilities API in YAML REST tests #108702

Merged
merged 3 commits into from
May 16, 2024

Conversation

mosche
Copy link
Contributor

@mosche mosche commented May 16, 2024

Document usage of Capabilities API in YAML REST tests (related to #108425 and #108678).

@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 16, 2024
@mosche mosche requested review from ldematte, thecoop and a team May 16, 2024 07:12
Copy link
Contributor

Documentation preview:

@elasticsearchmachine
Copy link
Collaborator

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

[[capabilities_check]]
=== Require or skip API capabilities

As opposed to <<cluster_features,cluster features>>, which are aimed at doing fast checks internally,
Copy link
Member

Choose a reason for hiding this comment

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

'... aimed at performing checks internal to Elasticsearch,' I think


Note: If planning to `skip` on capabilities, keep in mind this might lead to unexpected results in _mixed cluster_
tests. A test is only skipped if all nodes support the requested capabilities, in _mixed clusters_ this might not be
the case: you might randomly hit a node that actually supports what you intended to skip on.
Copy link
Member

Choose a reason for hiding this comment

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

I think this might need a bit more explanation - 'you might randomly hit a node that actually supports what you intended to skip on', what is the effect of this? Why does this matter when the capabilities check is across the whole cluster?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fair point, this is a tricky one... wdyt about the updated version:

Note: If planning to `skip` on capabilities, keep in mind this might lead to unexpected results in _mixed cluster_
tests. A test is only skipped if all nodes support the requested capabilities, in _mixed clusters_ this might not be
the case: such a cluster can consist of a mix of nodes where some support respective capabilities and others don't.
However, in that case, the test is *not* skipped and you might randomly hit one of the nodes that actually supports
what you intended to skip on. This might break your assumptions and fail the test. 

Copy link
Contributor

Choose a reason for hiding this comment

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

and you might randomly hit one of the nodes

Worth mentioning this is "if there is no internal coordination on the cluster feature"? Or would this makes the text to complicated?

Copy link
Member

Choose a reason for hiding this comment

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

I think we need some info like 'if not all nodes support the capabilities API itself, all capabilities are unsupported'

Copy link
Contributor Author

@mosche mosche May 16, 2024

Choose a reason for hiding this comment

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

I think we need some info like 'if not all nodes support the capabilities API itself, all capabilities are unsupported'

Do you really think that needs repetition @thecoop ? That should be clear based on the following just a few sentences above.

Only if every node in the cluster supports the requested path and method with all parameters and capabilities, the capabilities check passes successfully

Copy link
Member

Choose a reason for hiding this comment

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

I think it should be blindingly clear and obvious here why capabilities skips won't work on mixed clusters - a single paragraph we can point people to HERE HERE HERE. This will come up, and people will be confused.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@thecoop You're right, sorry I've misread. I'll change

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wondering, should we discourage using capabilities in skip sections? Or not even support it?

Copy link
Member

Choose a reason for hiding this comment

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

I think there is a use case (tests for previous behaviour)...

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, that's all right

@mosche mosche merged commit e4c6b0f into elastic:main May 16, 2024
15 checks passed
@mosche mosche deleted the yaml_capabilities_docs branch May 16, 2024 14:00
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.

4 participants