Skip to content

Conversation

@bluvulture
Copy link
Contributor

@bluvulture bluvulture commented Oct 24, 2025

This pull request updates the .github/workflows/reusable-codeception-tests-centralized.yaml workflow to add support for optionally enabling Elasticsearch as a service during tests. The main changes introduce a new input to control Elasticsearch activation and refactor the service configuration to conditionally start Elasticsearch based on this input.

Elasticsearch service configuration:

  • Added new input parameter ENABLE_ELASTICSEARCH (boolean, default false) to allow toggling Elasticsearch service in the workflow.
  • Refactored the elastic service definition to conditionally start Elasticsearch only when ENABLE_ELASTICSEARCH is set to true; otherwise, it uses a placeholder service (nginx:alpine). Environment variables and ports are also set conditionally.

Workflow inputs and job configuration:

  • Passed the new ENABLE_ELASTICSEARCH input into the job environment, allowing downstream steps and services to react to its value.

@bluvulture bluvulture requested a review from brusch as a code owner October 24, 2025 12:29
@bluvulture bluvulture requested review from Copilot and herbertroth and removed request for Copilot October 24, 2025 12:30
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 adds optional Elasticsearch support to the Codeception test workflow by introducing a conditional flag. The service can now be enabled only when needed, avoiding unnecessary resource allocation in test environments that don't require Elasticsearch.

Key Changes:

  • Added ENABLE_ELASTICSEARCH boolean input (defaults to false)
  • Replaced commented-out Elasticsearch configuration with conditional service that uses a dummy bash container when disabled

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
@bluvulture bluvulture requested a review from Copilot October 24, 2025 12:38
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

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.


Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@bluvulture
Copy link
Contributor Author

@bluvulture bluvulture requested a review from Copilot October 24, 2025 14:06
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

Copilot reviewed 1 out of 1 changed files in this pull request and generated 2 comments.


Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Comment on lines +139 to +140
ports:
- ${{ inputs.ENABLE_ELASTICSEARCH && inputs.PIMCORE_ELASTIC_SEARCH_HOST || '0' }}:9200
Copy link

Copilot AI Oct 24, 2025

Choose a reason for hiding this comment

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

Port mapping '0:9200' is invalid. When Elasticsearch is disabled, the service should not expose any ports. Consider using an empty array or omitting the ports configuration entirely when ENABLE_ELASTICSEARCH is false.

Suggested change
ports:
- ${{ inputs.ENABLE_ELASTICSEARCH && inputs.PIMCORE_ELASTIC_SEARCH_HOST || '0' }}:9200
ports: ${{ inputs.ENABLE_ELASTICSEARCH && format('[\"{0}:9200\"]', inputs.PIMCORE_ELASTIC_SEARCH_HOST) || '[]' }}

Copilot uses AI. Check for mistakes.
image: ${{ inputs.ENABLE_ELASTICSEARCH && format('elasticsearch:{0}', inputs.PIMCORE_ELASTIC_SEARCH_VERSION) || 'nginx:alpine' }}
ports:
- ${{ inputs.ENABLE_ELASTICSEARCH && inputs.PIMCORE_ELASTIC_SEARCH_HOST || '0' }}:9200
env: ${{ inputs.ENABLE_ELASTICSEARCH && fromJSON('{"discovery.type":"single-node","ES_JAVA_OPTS":"-Xms512m -Xmx512m","xpack.security.enabled":"true","xpack.security.authc.anonymous.roles":"superuser"}') || fromJSON('{}') }}
Copy link

Copilot AI Oct 24, 2025

Choose a reason for hiding this comment

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

[nitpick] The inline JSON for Elasticsearch environment variables reduces readability. Consider defining these as separate YAML key-value pairs or extracting to a reusable configuration block for better maintainability.

Suggested change
env: ${{ inputs.ENABLE_ELASTICSEARCH && fromJSON('{"discovery.type":"single-node","ES_JAVA_OPTS":"-Xms512m -Xmx512m","xpack.security.enabled":"true","xpack.security.authc.anonymous.roles":"superuser"}') || fromJSON('{}') }}
env:
discovery.type: ${{ inputs.ENABLE_ELASTICSEARCH && 'single-node' || '' }}
ES_JAVA_OPTS: ${{ inputs.ENABLE_ELASTICSEARCH && '-Xms512m -Xmx512m' || '' }}
xpack.security.enabled: ${{ inputs.ENABLE_ELASTICSEARCH && 'true' || '' }}
xpack.security.authc.anonymous.roles: ${{ inputs.ENABLE_ELASTICSEARCH && 'superuser' || '' }}

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants