Skip to content

Conversation

@cmaglie
Copy link
Member

@cmaglie cmaglie commented Dec 23, 2025

Motivation

Use callbacks on synchronous functions instead of the more complicated channels.
It also makes error handling more straightforward.

Change description

Additional Notes

Supersedes #91

Reviewer checklist

  • PR addresses a single concern.
  • PR title and description are properly filled.
  • Changes will be merged in main.
  • Changes are covered by tests.
  • Logging is meaningful in case of troubleshooting.

@cmaglie cmaglie requested a review from lucarin91 December 23, 2025 13:37
@cmaglie cmaglie self-assigned this Dec 23, 2025
@cmaglie cmaglie force-pushed the update-dont-use-iterators branch 2 times, most recently from d954a1a to ba3af71 Compare December 23, 2025 13:43
@cmaglie cmaglie force-pushed the update-dont-use-iterators branch from ba3af71 to 49bfbc5 Compare December 23, 2025 13:46
It is already broadcasted by the caller.
// It publishes events to subscribers during the upgrade process.
// It returns an error if the upgrade is already in progress or if the upgrade command fails.
func (s *Service) UpgradePackages(ctx context.Context, names []string) (<-chan update.Event, error) {
func (s *Service) UpgradePackages(ctx context.Context, names []string, eventCB func(update.Event)) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

I would define a type for the call back

Copy link
Contributor

Choose a reason for hiding this comment

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

I wll do in another PR

@lucarin91
Copy link
Contributor

@lucarin91 lucarin91 merged commit 5acdbae into main Dec 24, 2025
7 of 9 checks passed
@lucarin91 lucarin91 deleted the update-dont-use-iterators branch December 24, 2025 10:48
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.

2 participants