Skip to content

Rename IComponent result types - #797

Open
danth wants to merge 1 commit into
eclipse-score:mainfrom
etas-contrib:pin-rename-result
Open

Rename IComponent result types#797
danth wants to merge 1 commit into
eclipse-score:mainfrom
etas-contrib:pin-rename-result

Conversation

@danth

@danth danth commented Oct 15, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

I struggled to understand the original code because some values had multiple meanings, and different things were valid depending on the context.

Split into two results, one for de/activate and one for termination. The interesting changes are in icomponent.hpp.

Part of addressing #705 (comment)

WilliamRoebuck
WilliamRoebuck suggested changes Oct 15, 2026
View reviewed changes

@WilliamRoebuck WilliamRoebuck 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.

I like removing the error before/after ready distinction, that part was always quite confusing to me. Unfortunately though, a lot of the new changes have just become out of date yesterday since #738.

In summary, the whole async issue is no longer present, kWaiting can probably be removed. As far as I know:

  • tryReportCompletion is no longer needed. Existing calls should be replaced or removed
  • tryHandleTermination - should now only flag an error to the caller or return some sort of void/ignore result (no error yet). Should never return kSuccess. Terminations during startup are resolved at the end using the stored termination result member.
  • activate & deactivate - always runs synchronously and returns success or failure

This all probably allows process monitor to be simplified as well

There are also some leftover unit tests (several of the parameterized ones) that test various async termination scenarios, most of which can be removed.

@danth

danth commented Oct 15, 2026

Copy link
Copy Markdown
Member Author

In summary, the whole async issue is no longer present

This is good to hear. I think most of the dead code will be replaced anyway as part of #705, so in this pull request I'll only make the minimal edits needed to change the error types.

@danth

danth commented Oct 16, 2026

Copy link
Copy Markdown
Member Author

@WilliamRoebuck I've removed the async values from the enums. I'd appreciate if you could take another look :))

@danth
danth requested a review from WilliamRoebuck October 16, 2026 10:22
@danth
danth force-pushed the pin-rename-result branch from 90a525a to 3d86568 Compare October 16, 2026 10:59
@github-actions

github-actions Bot commented Oct 16, 2026

Copy link
Copy Markdown
Contributor

Documentation preview for this pull request is available at:
pr-797: https://eclipse-score.github.io/lifecycle/pr-797/

@danth danth mentioned this pull request Oct 16, 2026
Merged
MaciejKaszynski
MaciejKaszynski reviewed Oct 19, 2026
View reviewed changes
Comment thread score/launch_manager/src/daemon/src/process_group_manager/details/graph.cpp
Comment thread score/launch_manager/src/daemon/src/process_group_manager/details/icomponent.hpp Outdated
Comment thread score/launch_manager/src/daemon/src/process_group_manager/details/icomponent.hpp Outdated
Comment thread score/launch_manager/src/daemon/src/process_group_manager/details/process_info_node.cpp
@danth
danth requested a review from MaciejKaszynski October 19, 2026 15:39
WilliamRoebuck
WilliamRoebuck approved these changes Oct 20, 2026
View reviewed changes

@WilliamRoebuck WilliamRoebuck 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.

Looks great now, I only have two small suggestions

Comment thread score/launch_manager/src/daemon/src/process_group_manager/details/component_event.hpp
IdentifierHash node_identifier;
/// @brief Description of the error.
IComponent::ComponentError reason;
/// @brief Why the activation failed, either kFailure or kTimeout.

@WilliamRoebuck WilliamRoebuck Oct 20, 2026

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.

I think RequestResult should still be an expected (expected_blank I think) with an error code. Something that can be kSuccess being an error code seems wrong to me.

Comment thread score/launch_manager/src/daemon/src/process_group_manager/details/graph.cpp
@NicolasFussberger
NicolasFussberger deployed to workflow-approval October 22, 2026 12:45 — with GitHub Actions Active
@NicolasFussberger
NicolasFussberger deployed to workflow-approval October 22, 2026 12:45 — with GitHub Actions Active
@NicolasFussberger
NicolasFussberger deployed to workflow-approval October 22, 2026 12:45 — with GitHub Actions Active
@NicolasFussberger
NicolasFussberger deployed to workflow-approval October 22, 2026 12:45 — with GitHub Actions Active
@NicolasFussberger

NicolasFussberger commented Oct 23, 2026

Copy link
Copy Markdown
Contributor

Changes look good to me, though unit test compilation is currently failing on this branch

@danth
Rename IComponent result types
bc35e30 
The old result was difficult to understand because some values had
multiple meanings, and different things were valid depending on the
context.

Split into two results, one for de/activate and one for termination.
The interesting changes are in `icomponent.hpp`.
@danth
danth force-pushed the pin-rename-result branch from 492c494 to bc35e30 Compare October 23, 2026 10:30
@danth
danth requested a deployment to workflow-approval October 23, 2026 10:30 — with GitHub Actions Waiting
@danth
danth requested a deployment to workflow-approval October 23, 2026 10:30 — with GitHub Actions Waiting
@danth
danth requested a deployment to workflow-approval October 23, 2026 10:30 — with GitHub Actions Waiting
@danth
danth requested a deployment to workflow-approval October 23, 2026 10:30 — with GitHub Actions Waiting

This branch is waiting to be deployed

1 waiting deployment
workflow-approval — bc35e30e Waiting Oct 23, 2026 by danth via Build and test unit-tests-x86_64-qnx / approval #1718
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@NicolasFussberger NicolasFussberger Awaiting requested review from NicolasFussberger NicolasFussberger is a code owner
@pawelrutkaq pawelrutkaq Awaiting requested review from pawelrutkaq pawelrutkaq is a code owner
@ramceb ramceb Awaiting requested review from ramceb ramceb is a code owner
@FScholPer FScholPer Awaiting requested review from FScholPer FScholPer is a code owner
@antonkri antonkri Awaiting requested review from antonkri antonkri is a code owner
@anmittag anmittag Awaiting requested review from anmittag anmittag is a code owner
@MaciejKaszynski MaciejKaszynski Awaiting requested review from MaciejKaszynski MaciejKaszynski is a code owner
1 more reviewer
@WilliamRoebuck WilliamRoebuck WilliamRoebuck approved these changes
Reviewers whose approvals may not affect merge requirements

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

LCM - Lifecycle & Health FT
Status: Backlog

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@danth @NicolasFussberger @MaciejKaszynski @WilliamRoebuck