Skip to content

Conversation

@mashhurs
Copy link
Collaborator

@mashhurs mashhurs commented Oct 17, 2024

Description

This PR introduces changes aligned with Elasticsearch main branch which a big change, JDK 21 requirement.

Author's checklist

@mashhurs mashhurs changed the title Sync with es main branch Sync with Elasticsearch main branch Oct 17, 2024
@elastic elastic deleted a comment from elasticmachine Oct 18, 2024
@mashhurs mashhurs linked an issue Oct 23, 2024 that may be closed by this pull request
@mashhurs mashhurs mentioned this pull request Oct 24, 2024
@mashhurs mashhurs force-pushed the sync-with-es-main-branch branch from b53b26c to c11bbb9 Compare October 24, 2024 18:51
@mashhurs mashhurs marked this pull request as ready for review October 24, 2024 18:52
@elasticmachine
Copy link
Collaborator

💚 Build Succeeded

History

@mashhurs mashhurs requested a review from jsvd October 24, 2024 19:39
Copy link
Member

@jsvd jsvd 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
Member

@jsvd jsvd left a comment

Choose a reason for hiding this comment

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

should we move gradle.properties ES TREEISH to main?

@mashhurs
Copy link
Collaborator Author

mashhurs commented Oct 25, 2024

should we move gradle.properties ES TREEISH to main?

Not necessarily from functional point of view as we are able to enforce the ES treeish with env var. If we need it for a visibility (make sure we don't make any mistake while developing), I am thinking to update the GH PR template which should guide 8.x and future release and probably I will also add a note to a plugin publisher.
How do you think?

@jsvd
Copy link
Member

jsvd commented Oct 25, 2024

The issue I see is that, for a PR against the main branch of this plugin, for example, the main branch of ES is not being tested:

Resolved ELASTICSEARCH_TREEISH: 8.x
....
#31 34.81 > Task :downloadElasticsearchSourceZip
#31 34.81 Download https://github.com/elastic/elasticsearch/archive/refs/heads/8.x.zip

So we may need to force TREEISH to main in this branch, either through CI or the gradle.properties

@mashhurs
Copy link
Collaborator Author

mashhurs commented Oct 27, 2024

The issue I see is that, for a PR against the main branch of this plugin, for example, the main branch of ES is not being tested:

Resolved ELASTICSEARCH_TREEISH: 8.x
....
#31 34.81 > Task :downloadElasticsearchSourceZip
#31 34.81 Download https://github.com/elastic/elasticsearch/archive/refs/heads/8.x.zip

So we may need to force TREEISH to main in this branch, either through CI or the gradle.properties

I have a PR to separate branches and give better names. How do you think if I add there? I have validated on my local to cover your concern if it is about failure.
Commit: d89a233

  • Unit test job
  - label: ":hammer: Unit tests with LS 8.x-SNAPSHOT & ES main :docker:"
    command:
      - .buildkite/scripts/run_tests.sh
    env:
      ELASTIC_STACK_VERSION: "8.x"
      INTEGRATION: false
      SNAPSHOT: true
      ELASTICSEARCH_TREEISH: main

Local result:

logstash-1  | Finished in 4.37 seconds (files took 3.5 seconds to load)
logstash-1  | 74 examples, 0 failures
logstash-1  | 
logstash-1  | Randomized with seed 56776
logstash-1  | 
logstash-1 exited with code 0
  • Integration test job
  - label: ":hammer: Integration tests with LS 8.x-SNAPSHOT & ES main :docker:"
    command:
      - .buildkite/scripts/run_tests.sh
    env:
      ELASTIC_STACK_VERSION: "8.x"
      INTEGRATION: true
      LOG_LEVEL: "info"
      SECURE_INTEGRATION: true
      SNAPSHOT: true
      ELASTICSEARCH_TREEISH: main

Local result:

logstash-1       |       raises an exception when condition is met
logstash-1       | 
logstash-1       | Finished in 14.59 seconds (files took 3.35 seconds to load)
logstash-1       | 49 examples, 0 failures
logstash-1       | 
logstash-1       | Randomized with seed 61530
logstash-1       | 
logstash-1 exited with code 0

@jsvd
Copy link
Member

jsvd commented Oct 28, 2024

I'm fine with using #177 to improve the CI. LGTM

@jsvd jsvd merged commit c2e54e6 into elastic:main Oct 28, 2024
@mashhurs mashhurs deleted the sync-with-es-main-branch branch October 28, 2024 16:16
@mashhurs mashhurs self-assigned this Oct 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Upgrade JDK to 21 to sync with ES main branch.

3 participants