Skip to content

fix: allow StringParam for vpc_connector typing - #301

Closed
cabljac wants to merge 15 commits into
mainfrom
fix/vpc-connector-typing
Closed

fix: allow StringParam for vpc_connector typing#301
cabljac wants to merge 15 commits into
mainfrom
fix/vpc-connector-typing

Conversation

@cabljac

@cabljac cabljac commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

Description

Takes over #269 (all commits from @HassanBahati and @IzaakGough preserved). Widens the typing for the vpc_connector option to match the values already supported at runtime: str | Expression[str] | Sentinel | None, so StringParam and other string-valued params are type-correct.

The manifest generation logic is unchanged; it already serializes Expression[...] (including params) into CEL strings, so this is a typing alignment rather than a behavioral change.

Related Issues:

Fixes #219
Supersedes #269

Changes Made

  • Widened RuntimeOptions.vpc_connector in src/firebase_functions/options.py from str | Sentinel | None to str | Expression[str] | Sentinel | None.
  • Aligned the set_global_options signature for vpc_connector and vpc_connector_egress_settings with the RuntimeOptions field types.
  • Added parameterized tests in tests/test_options.py covering StringParam and other Expression values end to end (_asdict_with_global_options() and _endpoint() both produce the CEL string).
  • Added a tests/conftest.py autouse fixture clearing the global params registry between tests, needed for the new parameterized tests and a general test-isolation improvement.

Testing Evidence

Validated in isolation and via the MRE.

Tests Run (if applicable)

  • Unit tests
  • Integration tests
  • Manual testing

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

Copy link
Copy Markdown
Contributor

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 updates RuntimeOptions and set_global_options to allow vpc_connector to accept Expression[str] types (such as StringParam or ternary expressions), and adds corresponding unit tests. It also introduces pytest fixtures to clean up global options and parameters between tests. However, clearing the global params._params registry entirely in tests/conftest.py can delete module-level parameters from other test files, potentially causing test failures. It is recommended to capture and restore the registry's state instead of clearing it completely.

Comment thread tests/conftest.py
Comment on lines +25 to +30
@pytest.fixture(autouse=True)
def _cleanup_params():
"""Clear the global params registry so each test runs with a clean state."""
params._params.clear()
yield
params._params.clear()

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

high

Clearing params._params entirely between tests will delete any parameters declared at the module level in other test files (which are instantiated and registered when those test files are imported). This can cause subsequent tests that rely on those module-level parameters to fail with missing parameter errors or duplicate parameter errors if they are re-declared.

Instead of clearing the registry completely, we should capture the state of params._params before each test runs and restore it afterward. This preserves module-level parameters while still cleaning up any parameters declared dynamically inside individual tests.

Suggested change
@pytest.fixture(autouse=True)
def _cleanup_params():
"""Clear the global params registry so each test runs with a clean state."""
params._params.clear()
yield
params._params.clear()
@pytest.fixture(autouse=True)
def _cleanup_params():
"""Reset the global params registry to its pre-test state so tests don't leak params."""
original_params = params._params.copy()
yield
params._params.clear()
params._params.update(original_params)

@cabljac

cabljac commented Jul 23, 2026

Copy link
Copy Markdown
Contributor Author

Closing, #269 is the vehicle for this fix.

@cabljac cabljac closed this Jul 23, 2026
@cabljac
cabljac deleted the fix/vpc-connector-typing branch July 23, 2026 06:37
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.

Incorrect typing for the vpc_connector HttpsOption

3 participants