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

Update task names for rest compatiblity #75267

Merged
merged 9 commits into from
Sep 3, 2021

Conversation

jakelandis
Copy link
Contributor

@jakelandis jakelandis commented Jul 12, 2021

This commit updates two task names:

yamlRestCompatTest -> yamlRestTestV7CompatTest
transformV7RestTests -> yamlRestTestV7CompatTransform 

7 is the N-1 version and calculated, such that when 8 is
N-1 version the task names will be yamlRestTestV8CompatTest and
yamlRestTestV8CompatTransform

The motivation for yamlRestCompatTest -> yamlRestTestV7CompatTest is that
many projects have configured yamlRestCompatTest
but that configuration is specific to the N-1 version. For example,
if we black list tests when running compatibility with v7, we don't also
want to blacklist those tests when running compatibility with v8.

By introducing a version-specific identifier in the name, the task will not
even exist when bumping the version creating the need to (correctly) remove
the version-specific condition.

The motivation for transformV7RestTests -> yamlRestTestV7CompatTransform
is to provide more consistent naming.

The idea behind the naming is the main task people
are likely familiar with is :

yamlRestTest so we will use that as a base.
yamlRestTestV7CompatTest to run the version-specific compat tests
yamlRestTestV7CompatTransform to run the version-specific transformations for the compat tests

CI should be un-effected since since we introduced a lifecycle task
name checkRestCompat which is what CI should be configured to use.

@jakelandis jakelandis added :Delivery/Build Build or test infrastructure >non-issue labels Jul 12, 2021
@elasticmachine elasticmachine added the Team:Delivery Meta label for Delivery team label Jul 12, 2021
@elasticmachine
Copy link
Collaborator

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

Copy link
Contributor

@pgomulka pgomulka left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@mark-vieira mark-vieira left a comment

Choose a reason for hiding this comment

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

Can we change yamlRestTestV7Compat to yamlRestTestV7CompatTest? Generally the convention is that tasks that execute tests have a "test" suffix. This still aligns well (if not better) with the convention of yamlRestTestV7CompatTransform as well.

Copy link
Contributor

@breskeby breskeby left a comment

Choose a reason for hiding this comment

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

Lgtm once marks comment is Adressed and conflicts are resolved

@jakelandis
Copy link
Contributor Author

@mark-vieira - mind taking another look ?

The names have been updated.

Copy link
Contributor

@mark-vieira mark-vieira left a comment

Choose a reason for hiding this comment

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

Thanks, Jake. LGTM.

@jakelandis jakelandis merged commit 26dfe02 into elastic:master Sep 3, 2021
@jakelandis jakelandis deleted the compat_task_names branch September 3, 2021 16:26
wjp719 added a commit to wjp719/elasticsearch that referenced this pull request Sep 4, 2021
* master: (128 commits)
  Mute DieWithDignityIT (elastic#77283)
  Fix randomization in MlNodeShutdownIT (elastic#77281)
  Add target_node_name for REPLACE shutdown type (elastic#77151)
  [DOCS] Adds information about version compatibility headers (elastic#77096)
  Fix template equals when mappings are wrapped (elastic#77008)
  Fix TextFieldMapper Retaining a Reference to its Builder (elastic#77251)
  Move die with dignity to be a test module (elastic#77136)
  Update task names for rest compatiblity (elastic#75267)
  [ML] adjusting bwc serialization for elastic#77256 (elastic#77257)
  Move `index.hidden` from Static to Dynamic settings (elastic#77218)
  Handle cgroups v2 in `OsProbe` (elastic#77128)
  Choose postings format from FieldMapper instead of MappedFieldType (elastic#77234)
  Add segment sorter for data streams (elastic#75195)
  Update skip after backport (elastic#77212)
  [ML] adding new defer_definition_decompression parameter to put trained model API (elastic#77189)
  [ML] Fix bug in inference stats persister for when feature reset is called
  Only check replicas in cancelling existing recoveries. (elastic#60564)
  Format `AbstractFilteringTestCase` (elastic#77217)
  [DOCS] Fixes line breaks. (elastic#77248)
  Convert 'routing' values in REST API tests to strings
  ...

# Conflicts:
#	server/src/main/java/org/elasticsearch/cluster/metadata/DataStream.java
jakelandis added a commit that referenced this pull request Oct 12, 2021
This commit changes the sourceset name for REST API compatility testing
from yamlRestCompatTest to yamlRestTestV(N)Compat where (N) is the version
from which the tests are sourced. For example yamlRestTestV7Compat
or yamlRestTestV8Compat.

Two motivating reasons for this change.

    consitent naming - all compatible rest testing will now use a common
    prefix of yamlRestTestV7Compat where the task name to execute is
    yamlRestTestV7CompatTestand the transformation task is yamlRestTestV7CompatTransform
    It makes sense that corresponding source set follows this convention.

    Better support when the major version changes. Gradle uses the source
    set under the covers to execute the compatiblity tests. However, a user
    can add custom REST tests to this source set. By using dynamically named
    source sets, there should little concern of accidentally running the wrong
    version tests. Additionaly it is visualy easy to tell which version the tests are
    from.

This commit also removes the unecessary duplicated v7compat directory
and adds a duplicate checks to ensure that warn if  there are
duplicate test names.

related: #75267
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 >non-issue Team:Delivery Meta label for Delivery team v8.0.0-alpha2
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants