Skip to content

Conversation

@Marenz
Copy link
Contributor

@Marenz Marenz commented Aug 20, 2025

  • Support key signing
  • Consider dry_run status in dispatch merging

@github-actions github-actions bot added part:docs Affects the documentation part:tests Affects the unit, integration and performance (benchmarks) tests part:tooling Affects the development tooling (CI, deployment, dependency management, etc.) part:dispatcher Affects the high-level dispatcher interface labels Aug 20, 2025
@Marenz Marenz marked this pull request as ready for review August 21, 2025 09:13
Copilot AI review requested due to automatic review settings August 21, 2025 09:13
@Marenz Marenz requested a review from a team as a code owner August 21, 2025 09:13

This comment was marked as outdated.

Copy link
Contributor

@llucax llucax left a comment

Choose a reason for hiding this comment

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

Awesome to see the support for parallel dry-run actors! 🎉

Next time maybe split the deprecation of key in favor of auth_key in a separate commit, as it was sometimes confusing to have it mixed with the introduction of signing.

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 support for key signing and improves dry_run handling in the dispatch system. The changes introduce new authentication parameters and ensure that dispatches with different dry_run statuses are handled separately during merging operations.

  • Add support for key signing with new sign_secret parameter and deprecate key in favor of auth_key
  • Modify merge strategies to consider dry_run status when grouping dispatches
  • Update dependency version for frequenz-client-dispatch

Reviewed Changes

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

Show a summary per file
File Description
src/frequenz/dispatch/_dispatcher.py Add new authentication parameters (auth_key, sign_secret) and deprecate key parameter
src/frequenz/dispatch/_merge_strategies.py Include dry_run status in merge identity functions for both merge strategies
tests/test_managing_actor.py Update test client to support new authentication parameters
tests/test_frequenz_dispatch.py Add test to verify dry_run dispatches are not merged together
pyproject.toml Update frequenz-client-dispatch dependency version
RELEASE_NOTES.md Document new features and deprecation notice

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

) -> None:
assert server_url
assert key
assert key or auth_key
Copy link

Copilot AI Aug 21, 2025

Choose a reason for hiding this comment

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

The assertion logic doesn't match the validation in the actual Dispatcher class. The Dispatcher raises ValueError when both are provided or neither are provided, but this test only checks that at least one is provided. Consider aligning the test validation with the production code logic.

Suggested change
assert key or auth_key
# Ensure exactly one of key or auth_key is provided, matching Dispatcher logic
assert (key is not None) != (auth_key is not None)

Copilot uses AI. Check for mistakes.
@Marenz Marenz force-pushed the enhc branch 2 times, most recently from ea92532 to 2dd8031 Compare August 21, 2025 11:35
llucax
llucax previously approved these changes Aug 21, 2025
Copy link
Contributor

@llucax llucax left a comment

Choose a reason for hiding this comment

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

Just a couple of nitpick comments.

Comment on lines 231 to 239
if key is not None and auth_key is not None:
raise ValueError(
"Both 'key' and 'auth_key' are provided, please use only 'auth_key'."
)

if key is None and auth_key is None:
raise ValueError(
"Either 'key' or 'auth_key' must be provided to access the dispatch service."
)
Copy link
Contributor

Choose a reason for hiding this comment

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

It just occurred to me, but you could also use overloads to ensure this is called correctly when type-checking:

    @overload
    def __init__(
        self,
        *,
        microgrid_id: MicrogridId,
        server_url: str,
        key: str,
        auth_key: None = None,
        sign_secret: str | None = None,
        call_timeout: timedelta = timedelta(seconds=60),
        stream_timeout: timedelta = timedelta(minutes=5),
    ):
    @overload
    def __init__(
        self,
        *,
        microgrid_id: MicrogridId,
        server_url: str,
        key: None = None,
        auth_key: str,
        sign_secret: str | None = None,
        call_timeout: timedelta = timedelta(seconds=60),
        stream_timeout: timedelta = timedelta(minutes=5),
    ):

But might be overkill for this.

Marenz added 2 commits August 21, 2025 17:40
Signed-off-by: Mathias L. Baumann <[email protected]>
Dispatches with different `dry_run` values are now handled by separate actor instances.

This change modifies the `MergeByType` and `MergeByTypeTarget` merge strategies to include the `dry_run` status in the identity function. Previously, two dispatches with the same type and target, but different `dry_run` statuses, would be merged into a single actor instance. This could lead to unexpected behavior, as dry-run dispatches are intended for testing and should not affect production actors.

With this change, dispatches are only merged if they have the same type, target (for `MergeByTypeTarget`), and `dry_run` status.

Signed-off-by: Mathias L. Baumann <[email protected]>
@Marenz Marenz merged commit d1a1c20 into frequenz-floss:v0.x.x Aug 21, 2025
5 checks passed
@Marenz Marenz deleted the enhc branch August 21, 2025 15:46
@llucax llucax linked an issue Sep 18, 2025 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

part:dispatcher Affects the high-level dispatcher interface part:docs Affects the documentation part:tests Affects the unit, integration and performance (benchmarks) tests part:tooling Affects the development tooling (CI, deployment, dependency management, etc.)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Prevent dry_run dispatch from overriding live dispatch

2 participants