Skip to content

Conversation

Copy link
Contributor

Copilot AI commented Nov 3, 2025

Flagd uses FLAGD_SYNC_PORT for in-process sync service port, but the testbed only documented and tested FLAGD_PORT. This creates configuration inconsistency between flagd and provider tests.

Changes

  • Configuration table: Updated port option documentation to clarify that it uses FLAGD_PORT (for rpc) and FLAGD_SYNC_PORT (for in-process, with FLAGD_PORT as fallback)

  • New test scenarios: Added FLAGD_SYNC_PORT tests for the existing port option across configuration types:

    • Environment variable configuration using FLAGD_SYNC_PORT
    • Environment variable override with direct setting
    • Priority test verifying FLAGD_SYNC_PORT takes precedence over FLAGD_PORT when both are set
  • Backwards compatibility: Added @legacy-sync-port tagged scenarios testing FLAGD_PORT for in-process providers, enabling future deprecation

All new scenarios use the existing port option and are tagged @in-process for proper test filtering.

Example

@in-process
Scenarios: In-Process Sync Port
  | option | env             | type    | value |
  | port   | FLAGD_SYNC_PORT | Integer | 1234  |

@in-process @legacy-sync-port
Scenarios: In-Process Legacy Port (backwards compatibility)
  | option | env        | type    | value |
  | port   | FLAGD_PORT | Integer | 1234  |

@in-process
Scenario: FLAGD_SYNC_PORT takes priority over FLAGD_PORT
  Given an environment variable "FLAGD_SYNC_PORT" with value "9999"
  And an environment variable "FLAGD_PORT" with value "8888"
  When a config was initialized
  Then the option "port" of type "Integer" should have the value "9999"

Fixes #266

Original prompt

This section details on the original issue you should resolve

<issue_title>Flagd in_process env var should be FLAGD_SYNC_PORT instead of FLAGD_PORT</issue_title>
<issue_description>The cloud native way of configuring services is via environment variables, and they should be unified throughout the stack. We do have an issue with FLAGD_PORT, which is actually used twice, once for in-process and once for RPC mode on the provider. But flagd (to whom we connect) has two different environment variables. We should adapt to be compliant with the flagd way.

Most current test scenarios in the testbed rely on the default configuration, and only limited adaptation is required to address this issue.

What needs to change?

  • Generate or add specific tests focused on configuration where the environment variable for the in-process provider port is relevant.
  • Ensure these tests verify that both FLAGD_PORT and FLAGD_SYNC_PORT can be used as environment variables for in-process providers, to maintain backwards compatibility.
  • Existing tests that use the default configuration do not need to be changed.

Tagging for backwards compatibility

  • Add the @legacy-sync-port tag to all backwards compatibility scenarios relating to the in-process sync port.
  • This will enable us to easily deactivate these tests in the future if backwards compatibility support is no longer required.

Context

This issue is a sub-issue of open-feature/flagd#1573 and helps ensure correct and maintainable usage of environment variables in provider configuration tests.</issue_description>

Comments on the Issue (you are @copilot in this section)


💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.

Copilot AI changed the title [WIP] Flagd in_process env var should be FLAGD_SYNC_PORT instead of FLAGD_PORT Add FLAGD_SYNC_PORT support for in-process providers with backwards compatibility Nov 3, 2025
Copilot AI requested a review from aepfli November 3, 2025 05:58
Copilot finished work on behalf of aepfli November 3, 2025 05:58
@aepfli
Copy link
Member

aepfli commented Nov 3, 2025

@copilot we do not want to add another property called syncport we want to still use port for inprocess. We onlyn want to set this property via environmentvariable called FLAGD_SYNC_PORT and as a fallback for backwards compatibility FLAGD_PORT. we do not want to change the rest of the code and/or the variable name

Copy link
Contributor Author

Copilot AI commented Nov 3, 2025

@copilot we do not want to add another property called syncport we want to still use port for inprocess. We onlyn want to set this property via environmentvariable called FLAGD_SYNC_PORT and as ...

Fixed in commit 0bf3813. The implementation now:

  • Uses the existing port option (not a new syncPort option)
  • Tests port with FLAGD_SYNC_PORT environment variable for in-process providers
  • Tests port with FLAGD_PORT for backwards compatibility (tagged with @legacy-sync-port)
  • Updates documentation to clarify: FLAGD_PORT (rpc), FLAGD_SYNC_PORT (in-process, with FLAGD_PORT as fallback)

Copilot finished work on behalf of aepfli November 3, 2025 06:11
@aepfli
Copy link
Member

aepfli commented Nov 3, 2025

@copilot please add a test with both environment variables, to ensure that FLAGD_SYNC_PORT has priority

Copy link
Contributor Author

Copilot AI commented Nov 3, 2025

@copilot please add a test with both environment variables, to ensure that FLAGD_SYNC_PORT has priority

Added in commit be7a318. The new scenario sets both FLAGD_SYNC_PORT (value 9999) and FLAGD_PORT (value 8888) environment variables and verifies that the port option resolves to 9999, confirming FLAGD_SYNC_PORT takes priority.

Copilot finished work on behalf of aepfli November 3, 2025 06:16
@aepfli aepfli marked this pull request as ready for review November 3, 2025 06:19
@aepfli aepfli requested a review from a team as a code owner November 3, 2025 06:19
@aepfli aepfli changed the title Add FLAGD_SYNC_PORT support for in-process providers with backwards compatibility feat: Add FLAGD_SYNC_PORT support for in-process providers with backwards compatibility Nov 3, 2025
@open-feature open-feature deleted a comment from github-actions bot Nov 3, 2025
@open-feature open-feature deleted a comment from github-actions bot Nov 3, 2025
@aepfli
Copy link
Member

aepfli commented Nov 3, 2025

/gemini review

Copy link

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request introduces support for the FLAGD_SYNC_PORT environment variable for in-process providers, ensuring backward compatibility with FLAGD_PORT. The changes include updating the configuration documentation and adding a comprehensive set of Gherkin test scenarios to cover the new functionality, backward compatibility, environment variable overrides, and precedence rules. The changes are well-structured and the new tests are thorough. I have one suggestion to improve the readability of the updated documentation table.

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.

Flagd in_process env var should be FLAGD_SYNC_PORT instead of FLAGD_PORT

2 participants