Skip to content

refactor: implement builder pattern for OmnectDeviceServiceClient with lifecycle management#65

Merged
JanZachmann merged 2 commits intoomnect:mainfrom
JanZachmann:refactor-device-service-singleton
Nov 12, 2025
Merged

refactor: implement builder pattern for OmnectDeviceServiceClient with lifecycle management#65
JanZachmann merged 2 commits intoomnect:mainfrom
JanZachmann:refactor-device-service-singleton

Conversation

@JanZachmann
Copy link
Contributor

@JanZachmann JanZachmann commented Nov 10, 2025

Summary

This PR refactors OmnectDeviceServiceClient to implement a type-safe builder pattern with lifecycle management, improving encapsulation and maintainability.

Key Changes

  • Builder Pattern: Implemented as the only way to create OmnectDeviceServiceClient, with private struct fields
  • Optional Certificate Setup: Certificate creation is now optional and injectable via with_certificate_setup()
  • Optional Publish Endpoint: Publish endpoint registration is now optional via with_publish_endpoint()
  • Code Organization: Extracted service client creation into dedicated build_service_client() function
  • Simplified Certificate Module: Made CreateCertPayload public and removed intermediate WorkloadCertPayload
  • Version Bump: Updated to 1.0.6
  • Dockerfile Fix: Changed to copy entire src directory structure

Benefits

  • Better encapsulation through private fields and builder pattern
  • Clearer lifecycle management with explicit shutdown
  • Looser coupling via dependency injection (closures for setup)
  • More flexible configuration (both features are optional)
  • Easier testing with injectable dependencies

@JanZachmann JanZachmann force-pushed the refactor-device-service-singleton branch 4 times, most recently from 161520c to ad6b10b Compare November 11, 2025 14:14
…h lifecycle management

This refactoring improves the architecture of OmnectDeviceServiceClient by:

- Implementing a type-safe builder pattern as the only way to create the client
- Making certificate setup optional and injectable via closure
- Making publish endpoint registration optional
- Extracting client creation into dedicated build_service_client() function
- Bumping version to 1.0.6

Key changes:
- OmnectDeviceServiceClient fields are now private, enforcing builder usage
- Builder supports optional certificate setup via with_certificate_setup()
- Builder supports optional publish endpoint via with_publish_endpoint()
- Certificate payload (CreateCertPayload) is now public and reusable
- Removed intermediate WorkloadCertPayload struct for simplicity
- Shutdown is now a DeviceServiceClient trait method called explicitly from main
- Improved Dockerfile to copy entire src directory structure

This design provides better encapsulation, clearer lifecycle management,
and looser coupling between modules through dependency injection.

Signed-off-by: Jan Zachmann <50990105+JanZachmann@users.noreply.github.com>
@JanZachmann JanZachmann force-pushed the refactor-device-service-singleton branch from ad6b10b to 58fa57d Compare November 11, 2025 14:26
Copy link
Contributor

@empwilli empwilli left a comment

Choose a reason for hiding this comment

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

LGTM, minor nit

Comment on lines 124 to 133
pub struct OmnectDeviceServiceClientBuilder<F, Fut>
where
F: FnOnce(CreateCertPayload) -> Fut,
Fut: std::future::Future<Output = Result<()>>,
{
publish_endpoint: Option<PublishEndpoint>,
certificate_setup: Option<F>,
_phantom: std::marker::PhantomData<Fut>,
}

Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder whether we really need to make OmnectDeviceServiceClientBuilder generic over the Fut type. If not, we can drop the _phantom marker and simplify it a little bit.

If I understand the code correctly, Fut is only necessary as a bound for F for the result type? wouldn't it simplify things if we just had

pub struct OmnectDeviceServiceClientBuilder<F>
where
    F: FnOnce(CreateCertPayload) -> std::future::Future<Output = Result<()>>,
{
...

instead?

Alternatively, we should provide a default type for the Fut in the generic type declaration to simplify things at the call site.

Copy link
Contributor Author

@JanZachmann JanZachmann Nov 12, 2025

Choose a reason for hiding this comment

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

Good catch! I've simplified the builder by removing the Fut generic parameter and PhantomData. Changes:

  • Builder is no longer generic over F and Fut
  • Uses boxed trait objects for the certificate setup function
  • Split the type alias into two for better readability:
    • type CertSetupFuture = Pin<Box<dyn Future<Output = Result<()>>>>;
    • type CertSetupFn = Box<dyn FnOnce(CreateCertPayload) -> CertSetupFuture>;

The API remains identical for callers, and the small runtime cost (heap allocation + dynamic dispatch) is negligible since it only happens once during initialization. The improved simplicity is worth the trade-off.

Signed-off-by: Jan Zachmann <50990105+JanZachmann@users.noreply.github.com>
@JanZachmann JanZachmann merged commit 655a087 into omnect:main Nov 12, 2025
2 checks passed
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