Skip to content

Conversation

@Marenz
Copy link
Contributor

@Marenz Marenz commented Apr 24, 2025

fixes #242

WIP - not yet implemented in common

@github-actions github-actions bot added the part:protobuf Affects the protocol buffer definition files label Apr 24, 2025
@Marenz Marenz force-pushed the timeintervalfilter branch 2 times, most recently from 8ea286d to 0e4282a Compare May 7, 2025 09:45
@github-actions github-actions bot added the part:tooling Affects the development tooling (CI, deployment, dependency management, etc.) label May 7, 2025
Marenz and others added 2 commits May 7, 2025 12:33
Signed-off-by: Mathias L. Baumann <mathias.baumann@frequenz.com>
Signed-off-by: Mathias L. Baumann <mathias.baumann@frequenz.com>
@Marenz Marenz force-pushed the timeintervalfilter branch from 0e4282a to b7c0d11 Compare May 7, 2025 10:33
@Marenz Marenz added the cmd:skip-release-notes It is not necessary to update release notes for this PR label May 7, 2025
Signed-off-by: Mathias L. Baumann <mathias.baumann@frequenz.com>
@Marenz Marenz force-pushed the timeintervalfilter branch from b7c0d11 to b186af7 Compare May 7, 2025 10:44
@Marenz Marenz changed the title WIP - Use TimeIntervalFilter from api-common Use new definitions from api-common May 7, 2025
@Marenz Marenz marked this pull request as ready for review May 7, 2025 10:46
Copilot AI review requested due to automatic review settings May 7, 2025 10:46
@Marenz Marenz requested a review from a team as a code owner May 7, 2025 10:46
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 updates dependency versions and migrates proto definitions to use the new api-common layout.

  • Updated pyproject.toml to use a Git URL for frequenz-api-common and adjusted the protobuf version.
  • Revised dispatch.proto to import and use definitions from the new electrical_components and types namespaces.

Reviewed Changes

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

File Description
pyproject.toml Downgraded protobuf version and switched frequenz-api-common dependency to a Git URL.
proto/frequenz/api/dispatch/v1/dispatch.proto Updated imports and type references to the new api-common definitions.
Files not reviewed (1)
  • submodules/frequenz-api-common: Language not supported

# versions can't work with code that was generated with newer versions.
# https://protobuf.dev/support/cross-version-runtime-guarantee/#backwards
"protobuf == 5.29.4",
"protobuf == 5.29.3",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

THis is to match the common api version

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, interesting. I wonder if there is a better way to do this, it kind of sucks to have such a strong interdependence...

Copy link
Contributor

Choose a reason for hiding this comment

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

On the other hand, if this is about the protobuf message about runtime and build version mismatch, using different point releases of the same minor should work. Are you sure this is really needed?

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.

Are you sure you want to make the jump to an unrelated unreleased common API?

@Marenz
Copy link
Contributor Author

Marenz commented May 12, 2025

what do you mean unrelated?
I think technically this is on protocol level a no-op, so it should be okay?

@llucax
Copy link
Contributor

llucax commented May 12, 2025

Sorry, typo, I meant unreleased.

@llucax
Copy link
Contributor

llucax commented May 12, 2025

I think technically this is on protocol level a no-op, so it should be okay?

No, sadly this also translate to Python dependencies.

@llucax
Copy link
Contributor

llucax commented May 12, 2025

Which means this code won't be usable with the SDK unless the microgrid API client also uses exactly the same git hash as dispatch.

@llucax
Copy link
Contributor

llucax commented May 12, 2025

If we want to keep the development so we don´t need to do a huge update after the api-common release, maybe we can do it in a feature branch, so we don't block the development of the "stable" branch

@Marenz Marenz added the status:blocked Other issues must be resolved before this can be worked on label May 12, 2025
@Marenz Marenz closed this Jul 30, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cmd:skip-release-notes It is not necessary to update release notes for this PR part:protobuf Affects the protocol buffer definition files part:tooling Affects the development tooling (CI, deployment, dependency management, etc.) status:blocked Other issues must be resolved before this can be worked on

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Align TimeIntervalFilter in Dispatch API with Common API Specification and Trading API

2 participants