Skip to content
Merged
25 changes: 23 additions & 2 deletions REST_API_COMPATIBILITY.md
Original file line number Diff line number Diff line change
Expand Up @@ -154,9 +154,11 @@ if (request.getRestApiVersion() == RestApiVersion.V_7 && request.hasParam("limit

The above code checks the request's compatible version and if the request has the parameter in question. In this case the deprecation warning is not automatic and requires the developer to manually log the warning. `request.param` is also required since it consumes the value as to avoid the error of unconsumed parameters.

### Testing
### Testing Backwards Compatibility

The primary means of testing compatibility is via the prior major version's YAML REST tests. The build system will download the latest prior version of the YAML rest tests and execute them against the current cluster version. Prior to execution the tests will be transformed by injecting the correct headers to enable compatibility as well as other custom changes to the tests to allow the tests to pass. These customizations are configured via the build.gradle and happen just prior to test execution. Since the compatibility tests are manipulated version of the tests stored in Github (via the past major version), it is important to find the local (on disk) version for troubleshooting compatibility tests.
The primary means of testing compatibility is via the prior major version's YAML REST tests. The build system will download the latest prior version of the YAML rest tests and execute them against the current cluster version. For example if you are testing main versioned as 9.0.0 the build system will download the yaml tests in the 8.x branch and execute those against the current cluster version for 9.0.0.

Prior to execution the tests will be transformed by injecting the correct headers to enable compatibility as well as other custom changes to the tests to allow the tests to pass. These customizations are configured via the build.gradle and happen just prior to test execution. Since the compatibility tests are manipulated version of the tests stored in Github (via the past major version), it is important to find the local (on disk) version for troubleshooting compatibility tests.

The tests are wired into the `check` task, so that is the easiest way to test locally prior to committing. More specifically the task is called `yamlRestCompatTest`. These behave nearly identical to it's non-compat `yamlRestTest` task. The only variance is that the tests are sourced from the prior version branch and the tests go through a transformation phase before execution. The transformation task is `yamlRestCompatTestTransform`.

Expand All @@ -170,6 +172,25 @@ Since these are a variation of backward compatibility testing, the entire suite

In some cases the prior version of the YAML REST tests are not sufficient to fully test changes. This can happen when the prior version has insufficient test coverage. In those cases, you can simply add more testing to the prior version or you can add custom REST tests that will run along side of the other compatibility tests. These custom tests can be found in the `yamlRestCompatTest` sourceset. Custom REST tests for compatibility will not be modified prior to execution, so the correct headers need to be manually added.

#### 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


```groovy
tasks.named("yamlRestCompatTestTransform").configure({task ->
task.skipTest("range/20_synthetic_source/Date range", "date range breaking change causes tests to produce incorrect values for compatibility")
task.skipTest("indices.sort/10_basic/Index Sort", "warning does not exist for compatibility")
task.skipTest("search/330_fetch_fields/Test search rewrite", "warning does not exist for compatibility")
task.skipTestsByFilePattern("indices.create/synthetic_source*.yml", "@UpdateForV9 -> tests do not pass after bumping API version to 9 [ES-9597]")
})
```

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?


#### 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.


### Developer's workflow

There should not be much, if any, deviation in a developers normal workflow to introduce and back-port changes. Changes should be applied in main, then back ported as needed.
Expand Down