Skip to content

Conversation

@lordgamez
Copy link
Contributor

https://issues.apache.org/jira/browse/MINIFICPP-2675

Depends on github.com//pull/2066


Thank you for submitting a contribution to Apache NiFi - MiNiFi C++.

In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:

For all changes:

  • Is there a JIRA ticket associated with this PR? Is it referenced
    in the commit message?

  • Does your PR title start with MINIFICPP-XXXX where XXXX is the JIRA number you are trying to resolve? Pay particular attention to the hyphen "-" character.

  • Has your PR been rebased against the latest commit within the target branch (typically main)?

  • Is your initial contribution a single, squashed commit?

For code changes:

  • If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under ASF 2.0?
  • If applicable, have you updated the LICENSE file?
  • If applicable, have you updated the NOTICE file?

For documentation related changes:

  • Have you ensured that format looks appropriate for the output in which it is rendered?

Note:

Please ensure that once the PR is submitted, you check GitHub Actions CI results for build issues and submit an update to your PR as soon as possible.

@lordgamez lordgamez added depends-on-another-PR low-impact Test only or trivial change that's most likely not gonna introduce any new bugs labels Nov 12, 2025
@lordgamez lordgamez force-pushed the MINIFICPP-2675 branch 2 times, most recently from 9a3cbc2 to 53ade0b Compare November 13, 2025 12:56
@lordgamez lordgamez requested a review from Copilot November 17, 2025 12:27
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR migrates Elasticsearch integration tests from the centralized docker test framework to the new modular docker test framework. The migration involves moving container management, step definitions, and test scenarios from the shared integration test infrastructure to extension-specific test modules.

  • Adds new modular test infrastructure for Elasticsearch and Opensearch container management
  • Updates test scenarios to use the new modular framework patterns
  • Removes centralized Elasticsearch/Opensearch container and step implementations

Reviewed Changes

Copilot reviewed 16 out of 18 changed files in this pull request and generated 8 comments.

Show a summary per file
File Description
extensions/elasticsearch/tests/features/steps/steps.py Implements step definitions for Elasticsearch/Opensearch test scenarios
extensions/elasticsearch/tests/features/steps/elasticsearch_container.py Container class for managing Elasticsearch instances in tests
extensions/elasticsearch/tests/features/steps/opensearch_container.py Container class for managing Opensearch instances in tests
extensions/elasticsearch/tests/features/steps/elastic_base_container.py Base container class with shared functionality for both Elasticsearch and Opensearch
extensions/elasticsearch/tests/features/environment.py Test environment configuration using common hooks
extensions/elasticsearch/tests/features/elasticsearch.feature Updated test scenarios to use modular framework patterns
extensions/elasticsearch/tests/features/opensearch.feature Updated test scenarios to use modular framework patterns
extensions/elasticsearch/tests/features/resources/elasticsearch.yml Elasticsearch server configuration
extensions/elasticsearch/tests/features/resources/opensearch.yml Opensearch server configuration
docker/test/integration/features/steps/steps.py Removes centralized Elasticsearch/Opensearch step definitions
docker/test/integration/features/MiNiFi_integration_test_driver.py Removes centralized Elasticsearch/Opensearch helper methods
docker/test/integration/cluster/containers/ElasticsearchContainer.py Removed centralized container class
docker/test/integration/cluster/containers/OpensearchContainer.py Removed centralized container class
docker/test/integration/cluster/checkers/ElasticSearchChecker.py Removed centralized checker class
docker/test/integration/cluster/DockerTestCluster.py Removes Elasticsearch/Opensearch checker references
docker/test/integration/cluster/ContainerStore.py Removes Elasticsearch/Opensearch container acquisition logic
docker/test/integration/minifi/processors/PostElasticsearch.py Removed centralized processor class
docker/RunBehaveTests.sh Adds Elasticsearch tests to the list of extension tests to run

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copy link
Member

@szaszm szaszm left a comment

Choose a reason for hiding this comment

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

+1 after comments are addressed from earlier PRs

Comment on lines +67 to +68
'"cluster": ["all"],'
'"index": [{"names": ["my_index"],"privileges": ["all"]}]'
Copy link
Contributor

Choose a reason for hiding this comment

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

nitpicking, but these two lines could be indented (more) to make the structure clearer

assert context.containers["elasticsearch"].create_doc_elasticsearch("my_index", "preloaded_id") or context.containers["elasticsearch"].log_app_output()


@given(u'an ElasticsearchCredentialsControllerService is set up with Basic Authentication')
Copy link
Contributor

Choose a reason for hiding this comment

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

the u'' prefixes can be removed from this file, too

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

Labels

depends-on-another-PR low-impact Test only or trivial change that's most likely not gonna introduce any new bugs

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants