Skip to content

Conversation

@john-wagster
Copy link
Contributor

As part of the work on a date range indexing breaking fix I ran some gotchas where we needed a series of steps to get the breaking changes and yaml tests working appropriately. I tried to capture the gotchas of that process here in the docs.

Related to: #112258

@john-wagster john-wagster added >docs General docs changes :Search Foundations/Mapping Index mappings, including merging and defining field types v9.0.0 labels Oct 2, 2024
@elasticsearchmachine elasticsearchmachine added Team:Docs Meta label for docs team Team:Search Foundations Meta label for the Search Foundations team in Elasticsearch labels Oct 2, 2024
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-docs (Team:Docs)

@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-search-foundations (Team:Search Foundations)

@thecoop thecoop requested review from a team October 3, 2024 08:03
@thecoop thecoop added the :Delivery/Build Build or test infrastructure label Oct 3, 2024
@elasticsearchmachine elasticsearchmachine added the Team:Delivery Meta label for Delivery team label Oct 3, 2024
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-delivery (Team:Delivery)

@leemthompo
Copy link
Contributor

@john-wagster looks like this got lost in the mists of time? :)

@john-wagster
Copy link
Contributor Author

@leemthompo it sure did; thanks for the heads up on this. Don't know how I missed it. Will review and revive.

@john-wagster
Copy link
Contributor Author

@elasticmachine update branch

@john-wagster
Copy link
Contributor Author

@andreidan @thecoop can I get a review and +1 on this PR related to improving the backwards compat docs based on our experiences with trying to get the breaking date range fixes in last year

Copy link
Contributor

@andreidan andreidan left a comment

Choose a reason for hiding this comment

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

Apologies for the delay @john-wagster Thanks for the nudge and thanks for putting this together !

I left a few questions/suggestions

})
```

Minimally once this skip test is in place, the correct `skip` and `requires` checks will need to be backported. Once backported the yaml compatibility tests should now work without the skip in the `build.gradle` file, and it should be removed. Consideration for whether tests should be cleaned up and changed based on how the breaking changes were backported is at the discretion of the team making the changes.
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we expand this paragraph with concrete versions? i.e. given the 9.0 and 8.latest example, the backport goes to 8.latest etc

Copy link
Contributor Author

Choose a reason for hiding this comment

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

tried to add in refs to 9.0.0 and 8.latest; let me know if that helps at all?


#### Breaking Changes

It is possible to be in a state where you have intentionally made a breaking change and the compatibility tests will fail irrespective of checks for `skip` or `requires` cluster or test features in the current version. In this state, assuming the breaking changes are reasonable and agreed upon by the breaking change committee, the correct behavior is to skip the test in the `build.gradle`. For example, if you make a breaking change that causes the `range/20_synthetic_source/Date range` to break then this test can be disabled temporarily in this file `rest-api-spec/build.gradle` like within this snippet:
Copy link
Contributor

Choose a reason for hiding this comment

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

should we expand on where and when this skip should be added? (i.e. 9.0 or 8.latest release branch)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

same here I took this whole section and tried to better flesh out examples with versions let me know if that helped at all


#### Test Features

Both cluster and test features exist. Cluster features are meant for new capability and test features can specifically be used to gate and manage `skip` and `requires` yaml test operations. Specifically in the context of backward compatibility and backporting, cluster and test features must be used consistently. When backporting and using these features they can not overlap in name and must be consistent when backported so that clusters built with these features are compatible.
Copy link
Contributor

Choose a reason for hiding this comment

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

Cluster features are meant for new capability ... cluster and test features must be used consistently

I believe it would be helpful to either expand on their intended purpose or provide links to additional documentation. The current paragraph could benefit from clearer guidance on when and how to implement these effectively."

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@andreidan honestly I don't know if we have documentation for that (if we do I haven't seen it). And since test features are relatively new I'm not sure we have official guidance somewhere.

@thecoop do you have more context there and/or additional docs we could link to? What do you think about the current state of this paragraph?

I tried to add an additional sentence to provide more context here based on my understanding of the difference between the two. Let me know what ya'll think.

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 the paragraph is a bit confusing. Capabilities and features have different use cases. The docs in docs/internal/Versioning.md cover the different uses. I would just link to that doc, and only say here that capabilities and feature names need to be consistent across branches for the same functionality.

@leemthompo
Copy link
Contributor

leemthompo commented Jan 29, 2025

[EDIT: This PR isn't impacted by the docs freeze because it's not editing an asciidoc file]

@john-wagster
Copy link
Contributor Author

@elasticmachine update branch

Copy link
Contributor

@andreidan andreidan left a comment

Choose a reason for hiding this comment

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

Thanks for iterating on this John.
Just left a couple more suggestions.


#### Test Features

Both cluster and test features exist. Cluster features are meant for new capability and test features can specifically be used to gate and manage `skip` and `requires` yaml test operations. For more information, see `docs/internal/Versioning.md`. Specifically in the context of backward compatibility and backporting, cluster and test features must be used consistently across branches. When backporting and using these features they can not overlap in name and must be consistent when backported so that clusters built with these features are compatible.
Copy link
Contributor

Choose a reason for hiding this comment

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

Shall we link to Versioning.md? (there's an example here on how to link )

Copy link
Contributor Author

Choose a reason for hiding this comment

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

used a markdown compatible link but should be good; let me know if you have concerns about what I threw in there as a link

@breskeby breskeby removed the request for review from a team February 21, 2025 09:55
@john-wagster
Copy link
Contributor Author

@elasticmachine update branch

@john-wagster
Copy link
Contributor Author

@thecoop @andreidan just bumping for ya'll to take another look when you get a chance and approve if it looks good at this point.

Copy link
Contributor

@andreidan andreidan left a comment

Choose a reason for hiding this comment

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

Thanks for iterating on this Wags.

Left a potential rephrasing suggestion. Hope it makes sense.

})
```

Minimally once this skip test is in place, the correct `skip` and `requires` checks will need to be backported to say 8.latest. Once backported, the test can be re-enabled in 9.0.0 by removing the `skipTest`. Consideration for whether tests should be cleaned up and changed based on how the breaking changes were backported is at the discretion of the team making the changes. As an example in 9.0.0 a `requires` should be added for new tests to validate the changed api or output and `skip` added for the old tests which break in 9.0.0. Then in backport say to 8.latest the `requires` check should be added to the existing tests with the old behavior so they no longer break when running bwc or upgrade tests from 9.0.0 to 8.latest.
Copy link
Contributor

Choose a reason for hiding this comment

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

This is still leaving me wondering what I need to do here (sorry, perhaps it's a "me" problem - bwc is mind twisting :) )

Just so I understand correctly:

When skipping a test temporarily in 9.0.0, we have to implement the proper skip and requires conditions to previous branches, such as 8.latest. After these conditions are implemented in 8.latest, you can re-enable the test in 9.0.0 by removing the skipTest condition.

The team implementing the changes can decide how to clean up or modify tests based on how breaking changes were backported. e.g.:

In 8.latest:

  • Add requires/ skip conditions to existing tests that check the old behavior. This prevents those tests from failing during backward compatibility or upgrade testing from 8.latest to 9.0.0

In 9.0.0:

  • Add requires conditions for new tests that validate the updated API or output format
  • Add skip conditions for older tests that would break in 9.0.0

Is this a correct summary? If so, can we rephrase this paragraph to something similar ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@andreidan completely fine with the rewrite here and agree it helps; updated to reflect your suggestion (with only minor changes to the wording). Let me know if you feel good about that update and I'll wait a bit for any additional feedback and then merge.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for iterating here. LGTM

Copy link
Contributor

@andreidan andreidan left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for iterating on this Wags

@john-wagster john-wagster merged commit 207da8a into elastic:main Mar 20, 2025
15 checks passed
afoucret pushed a commit to afoucret/elasticsearch that referenced this pull request Mar 21, 2025
…es - Yaml Compat Doc Updates (elastic#113964)

updating yaml compat docs
smalyshev pushed a commit to smalyshev/elasticsearch that referenced this pull request Mar 21, 2025
…es - Yaml Compat Doc Updates (elastic#113964)

updating yaml compat docs
omricohenn pushed a commit to omricohenn/elasticsearch that referenced this pull request Mar 28, 2025
…es - Yaml Compat Doc Updates (elastic#113964)

updating yaml compat docs
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

:Delivery/Build Build or test infrastructure >docs General docs changes :Search Foundations/Mapping Index mappings, including merging and defining field types Team:Delivery Meta label for Delivery team Team:Docs Meta label for docs team Team:Search Foundations Meta label for the Search Foundations team in Elasticsearch v9.1.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants