Skip to content

Align component target naming with monitoring API #295

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Marenz wants to merge 1 commit into
base: v1.x.x
Choose a base branch
from
Open

Align component target naming with monitoring API #295

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 9 additions & 1 deletion RELEASE_NOTES.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,4 +2,12 @@

## Summary

This is the v1.0.0 release. It is equivalent to v1.0.0-rc3 and introduces a period of API stabilization.
## What's Changed

* Renamed component target message types and fields for consistency with monitoring API:
* `IdSet` → `ElectricalComponentIdSelector`
* `CategorySet` → `ElectricalComponentCategorySelectorDeprecated`
* `CategoryTypeSet` → `ElectricalComponentCategorySelector`
* `CategoryAndType` → `ComponentCategory`
* `component_categories` → `component_categories_deprecated` (deprecated field)
Copy link
Contributor

@llucax llucax Nov 19, 2025

Choose a reason for hiding this comment

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

Why the renaming here, just to make it more explicit that it is deprecated? This will make it a breaking change in the generated code (even if the wire format stays compatible).

I guess since we are introducing many breaking changes to the generated code it is not a big deal, but I still wonder...

Copy link
Contributor Author

@Marenz Marenz Nov 19, 2025

Choose a reason for hiding this comment

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

I mean, all changes of this PR will break the generated code-dependent projects, unfortunately, yes.
And All changes are only for syncing/better understandability.

Copy link

@thomas-nicolai-frequenz thomas-nicolai-frequenz Nov 20, 2025

Choose a reason for hiding this comment

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

I don't think we should worry much about breaking changes at this point in time. Better now than later.

Copy link
Contributor

@llucax llucax Nov 21, 2025

Choose a reason for hiding this comment

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

Yeah, not a big issue, we can also deal with the breaking change at other level of the python abstraction.

Still for me it sounds weird to document a field is deprecated in the field name, there is even a protobuf way to do it: int32 old_field = 6 [deprecated = true]; (not sure what the effect is on generated code though.

Copy link

@thomas-nicolai-frequenz thomas-nicolai-frequenz Nov 22, 2025

Choose a reason for hiding this comment

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

Still for me it sounds weird to document a field is deprecated in the field name, there is even a protobuf way to do it: int32 old_field = 6 [deprecated = true]; (not sure what the effect is on generated code though.

Yeah agreed. We could also release a new version if required. Otherwise I'd suggest to just remove the field after all.

Copy link

@ktickner ktickner Nov 24, 2025

Choose a reason for hiding this comment

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

I think the point is less to document the deprecation, and more to release the property name to be used by the updated API

Copy link

@ktickner ktickner Nov 24, 2025

Choose a reason for hiding this comment

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

evidenced by the fact that Marenz is using the deprecated option already

Copy link
Contributor

@llucax llucax Nov 27, 2025

Choose a reason for hiding this comment

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

Yeah, but that change is a breaking change for users of the generate code, which is not great. So I think it is worth discussing. If we need to go fast here, I'm OK with merging as it is, but I don't think this is a good approach moving forward.

* `component_categories_types` → `component_categories`
20 changes: 10 additions & 10 deletions proto/frequenz/api/dispatch/v1/dispatch.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -388,13 +388,13 @@ message DispatchFilter {
message TargetComponents {
// Wrapper for controlling dispatches with a set of component IDs
// Required as we can't use `repeated` directly in a `oneof`
message IdSet {
message ElectricalComponentIdSelector {
// Set of component IDs
repeated uint64 ids = 1;
}

// Deprecated: Use `CategoryTypeSet` instead
message CategorySet {
// Deprecated: Use `ElectricalComponentCategorySelector` instead
message ElectricalComponentCategorySelectorDeprecated {
option deprecated = true;
// Set of component categories
repeated frequenz.api.common.v1alpha8.microgrid.electrical_components
Expand All @@ -403,15 +403,15 @@ message TargetComponents {

// Wrapper for controlling dispatches with a set of component categories and optional types
// Required as we can't use `repeated` directly in a `oneof`
message CategoryTypeSet {
message ElectricalComponentCategorySelector {
// Set of component categories
repeated CategoryAndType categories = 1;
repeated ComponentCategory categories = 1;
}

// A tuple of a required category and an optional type
// If a type is specified, it must be a valid type for the given category and
// only components of that type will be targeted.
message CategoryAndType {
message ComponentCategory {
// The category of the target component (required)
frequenz.api.common.v1alpha8.microgrid.electrical_components
.ElectricalComponentCategory category = 1;
Expand All @@ -437,15 +437,15 @@ message TargetComponents {

oneof components {
// Set of component IDs
IdSet component_ids = 1;
ElectricalComponentIdSelector component_ids = 1;

// Deprecated: Component categories
// Use `CategoryTypeSet` instead
// Use `ElectricalComponentCategorySelector` instead
// In future versions, this field will be removed.
CategorySet component_categories = 2 [deprecated = true];
ElectricalComponentCategorySelectorDeprecated component_categories_deprecated = 2 [deprecated = true];

// Component categories with optional types
CategoryTypeSet component_categories_types = 3;
ElectricalComponentCategorySelector component_categories = 3;
Comment on lines -445 to +448
Copy link
Contributor

@llucax llucax Nov 27, 2025

Choose a reason for hiding this comment

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

So, looking at this, I feel this is a bit backwards. If we want to keep backwards compatibility, it would make more sense have a new name for the new, backwards incompatible, field:

    ElectricalComponentCategorySelector component_categories = 2 [deprecated = true];

    ElectricalComponentCategorySelector2 component_categories2 = 3;

This aligns with:

Then in v2 we can rename ElectricalComponentCategorySelector2 and component_categories2 to remove the suffix.

This way we truly keep backwards compatibility.

I know for this case in particular it kind of defeats the purpose if we want to actually unify the names, but this might have been a sign that we did the 1.0 release too early.

I this change is really important, then I guess we can go for the approach in the PR or even going straight for a v2, but I don't think this is the way we should move forward. We are sacrificing backwards-compatibility only for a pretty name. Making breaking changes is very disruptive.

}
}

Expand Down