Restructure Microcluster packages by roosterfish · Pull Request #534 · canonical/microcluster

roosterfish · 2025-11-12T12:48:13Z

This started out small but after some changes I realized we have to properly hide structs behind interfaces to not anymore return datatypes which aren't usable by the user as they are placed inside the internal package.

With this PR I tried to remove all the pain points we have identified over the last years. Furthermore a user of Microcluster can now properly mock all the types to allow testing their DB and API handler logic.

More description and commit messages pending.

Signed-off-by: Julian Pelizäus <julian.pelizaeus@canonical.com>

tomponline · 2025-11-12T12:50:02Z

This started out small but after some changes I realized we have to properly hide structs behind interfaces to not anymore return datatypes which aren't usable by the user as they are placed inside the internal package.

More description and commit messages pending.

I've not looked at the code, but if these structs need to be accessible to the caller why are they being moved to internal and then interfaces being used to solve the problem?

roosterfish · 2025-11-12T12:54:03Z

I've not looked at the code, but if these structs need to be accessible to the caller why are they being moved to internal and then interfaces being used to solve the problem?

One aspect is that we don't have to expose code which is not intended for public use. Another (most important in my opinion) is that you can mock the types for unit testing. Just updated the initial comment.

tomponline · 2025-11-12T12:55:59Z

I've not looked at the code, but if these structs need to be accessible to the caller why are they being moved to internal and then interfaces being used to solve the problem?

One aspect is that we don't have to expose code which is not intended for public use. Another (most important in my opinion) is that you can mock the types for unit testing. Just updated the initial comment.

Can you define the types externally to internal and then use them from both the /internal functions and the call sites (like shared.api)?

roosterfish · 2025-11-12T13:40:27Z

Can you define the types externally to internal and then use them from both the /internal functions and the call sites (like shared.api)?

Yes that is the idea. This PR introduces a microcluster/types package which doesn't import "back" to prevent cycles (same as shared.api).
But in order to do this lots of structs required to move some of their fields from structs to interfaces to prevent import cycles.
Also a pattern this PR gets rid of is the following, exposing internal types by redefining them publicly, see https://github.com/canonical/microcluster/blob/v3/state/state.go.

I'll rework both commits and PR description to be more concrete about what is happening but just wanted to push so that I have something to discuss with @markylaing in our next meet.

Copilot

Pull Request Overview

This PR restructures Microcluster packages to improve API design by moving types from internal packages to public ones and introducing interfaces to enable better testing and mocking. The refactoring hides implementation details behind interfaces while making core types publicly accessible.

Key changes:

Relocated types from rest/, state/, and internal packages to microcluster/types/
Introduced interfaces (State, DB, OS, Client, Connector) to hide internal implementations
Converted client methods to standalone functions for better interface compliance
Updated all import paths throughout the codebase

Reviewed Changes

Copilot reviewed 98 out of 103 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
`microcluster/types/*.go`	New public type definitions moved from internal packages including State, DB, OS interfaces and core types
`rest/`, `state/` packages	Deleted type alias files, functionality moved to microcluster/types
`internal/rest/client/client.go`	Client methods converted to standalone functions, added HTTP() accessor
`internal/state/state.go`	Updated to use new types package, added Connect() and Member() methods
`internal/db/`	Updated schema and query imports to use public microcluster/db package
`internal/sys/os.go`	OS struct fields made private, added public accessor methods
`microcluster/app.go`	Updated to use new types and standalone client functions
Test files	Package names updated to reflect new structure (e.g., `query_test` → `db_test`)

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

tomponline · 2025-11-12T13:56:08Z

Can you define the types externally to internal and then use them from both the /internal functions and the call sites (like shared.api)?

Yes that is the idea. This PR introduces a microcluster/types package which doesn't import "back" to prevent cycles (same as shared.api). But in order to do this lots of structs required to move some of their fields from structs to interfaces to prevent import cycles. Also a pattern this PR gets rid of is the following, exposing internal types by redefining them publicly, see https://github.com/canonical/microcluster/blob/v3/state/state.go.

I'll rework both commits and PR description to be more concrete about what is happening but just wanted to push so that I have something to discuss with @markylaing in our next meet.

OK thanks for explaining. I would just say be cautious in using too many Interfaces and check they are truly needed because there is a cost to using them. I myself have used interfaces to break import cycles in the past so I understand what you are doing.

Also ensure that all endpoints are covered by a client func in the app construct.

markylaing · 2025-11-17T10:35:24Z

Few points from meeting:

Summary of which endpoints have been moved where and why. Anything /internal should be something that a client will never call. Any changes need careful consideration for upgrades from v2 to v3. Mixed versions could be breaking if APIs have moved around.
Package moves all good 👍
/internal/sql should not be on TLS listener (unix only).

tomponline · 2025-11-17T10:50:09Z

/internal/sql should not be on TLS listener (unix only).

So important!

This is the first PR taken from a single commit in #534. The final package layout will be different but to allow reviewing the changes in smaller chunks I will split this PR by its commits. It consolidates the DB funcs and ensures the ones that should be used externally are public and not in the `internal` package. After the restructuring all the DB related functionality will be under the `microcluster/db` package alongside the app construct which is located in `microcluster`. See https://github.com/canonical/microcluster/pull/534/files

kadinsayani

The new package structure LGTM overall. Shall we also update the Makefile update-schema target in this PR?

❯ make update-schema
go generate ./cluster/...
pattern ./cluster/...: lstat ./cluster/: no such file or directory
make: *** [Makefile:66: update-schema] Error 1

roosterfish · 2025-12-02T16:21:21Z

The new package structure LGTM overall. Shall we also update the Makefile update-schema target in this PR?

Good catch. We should drop this overall as we agreed on not anymore using lxd-generate and instead maintain the few queries manually.

This is the second PR taken from a single commit in #534. The final package layout will be different but to allow reviewing the changes in smaller chunks I will split this PR by its commits. In this PR types which might end up on the user's end through the various channels (client funcs, app funcs, API handlers) are now moved to the central `microcluster/*` package which is the regular entry point for Microcluster users to derive an app construct.

This is the third PR taken from a single commit in #534. The final package layout will be different but to allow reviewing the changes in smaller chunks I will split this PR by its commits. In this PR the state directory utils (internal `sys` package) are not anymore returned directly from the state but are now hidden behind a common `OS` interface inside `microcluster/types` package. As the state is exposed to API handlers written by downstream users, this allows proper mocking. We should expose this to allow easy access to the daemon's config and certificates where necessary.

This is the fourth PR taken from a single commit in #534. The final package layout will be different but to allow reviewing the changes in smaller chunks I will split this PR by its commits. In this PR two new `Client` and `Connector` interfaces are added to address all use cases when it comes to cluster wide HTTP connections. In addition we now have app level funcs for all user facing operations which previously required going to a separate client package. Furthermore the specific query functions are now moved to `internal/` as they should not be exposed directly to the end user.

This is the fifth PR taken from a single commit in #534. The final package layout will be different but to allow reviewing the changes in smaller chunks I will split this PR by its commits. In this PR the opinionated `api.URL` type from LXD (which embeds the std libs `url.URL`) is replaced with the std libs `url.URL` to lower the amount of deps on LXD's `shared` package. But we still use it to allow fast crafting of URLs internally. Also it simplifies many statements that require access to an URL's `Host` as there is one less level of struct nesting.

This is the sixth PR taken from a single commit in #534 (even though in this case the actual changes are very different from the original commit). The final package layout will be different but to allow reviewing the changes in smaller chunks I will split this PR by its commits. In this PR the `Remotes` func from the `State` interface is replaced with an interface to not anymore return an implementation of the truststore with types located inside the `internal/` package. Furthermore the naming is made a bit more consistent: * `Store` represents a truststore * `Remote` represents an entry in the truststore The initial idea was to just hide the truststore access. However it's a very inexpensive way of querying for the existence and details of cluster members as the truststore is already located in memory.

This is the seventh PR taken from a single commit in #534 (even though in this case the actual changes are very different from the original commit). The final package layout will be different but to allow reviewing the changes in smaller chunks I will split this PR by its commits. In this PR the extensions package is made public so that the actual types can be accessed by importers of Microcluster.

Can only be merged after (won't build before): * #607 This is the eigth PR taken from a single commit in #534 (even though in this case the actual changes are very different from the original commit). The final package layout will be different but to allow reviewing the changes in smaller chunks I will split this PR by its commits. In this PR the DB interface is made public. It's returned by the public `State` interface and should therefore not be located inside the `internal/` package.

roosterfish added 11 commits October 31, 2025 15:28

rebase make DB stuff externally available

e087f5c

move all to microcluster (rebase)

9254eca

Hide OS behind an interface

46fcbf5

Signed-off-by: Julian Pelizäus <julian.pelizaeus@canonical.com>

create client and clients interfaces

5ff4a4f

Signed-off-by: Julian Pelizäus <julian.pelizaeus@canonical.com>

return std libraries url type from state

6c7148c

drop remotes from public state

847a88e

move extensions to public type

9b7e8be

move DB interface to public types

be35130

move hook to public types and drop odd public state package

0b0c135

move rest related types to std types package for further simplification

2cfb9c2

merge response package into std types

6b37ab5

roosterfish requested a review from Copilot November 12, 2025 13:43

Copilot started reviewing on behalf of roosterfish November 12, 2025 13:43 View session

Copilot finished reviewing on behalf of roosterfish November 12, 2025 13:44

Copilot AI reviewed Nov 12, 2025

View reviewed changes

move all endpoints under /core/internal

e355485

Also ensure that all endpoints are covered by a client func in the app construct.

roosterfish mentioned this pull request Nov 26, 2025

Restructure: Consolidate db package and fix regression #545

Merged

roosterfish mentioned this pull request Dec 2, 2025

Restructure (2): Consolidate packages and move to microcluster #552

Merged

kadinsayani reviewed Dec 2, 2025

View reviewed changes

roosterfish mentioned this pull request Dec 2, 2025

Restructure (3): Hide state directory utils behind interface #553

Merged

roosterfish mentioned this pull request Jan 12, 2026

Restructure (4): HTTP client interface with cluster wide queries #582

Merged

roosterfish mentioned this pull request Jan 16, 2026

Restructure (5): Use std libraries URL type only #583

Merged

roosterfish mentioned this pull request Jan 20, 2026

Restructure (6): Store interface for local truststore access #590

Merged

This was referenced Feb 3, 2026

Restructure (7): Make extensions package public #607

Merged

Restructure (8): Make DB interface public #608

Merged

roosterfish mentioned this pull request Feb 5, 2026

Restructure (9): Make State interface and Hooks public #610

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Restructure Microcluster packages#534

Restructure Microcluster packages#534
roosterfish wants to merge 12 commits intocanonical:v3from
roosterfish:move_db_packages

roosterfish commented Nov 12, 2025 •

edited

Loading

Uh oh!

tomponline commented Nov 12, 2025

Uh oh!

roosterfish commented Nov 12, 2025

Uh oh!

tomponline commented Nov 12, 2025

Uh oh!

roosterfish commented Nov 12, 2025

Uh oh!

Copilot AI left a comment

Uh oh!

tomponline commented Nov 12, 2025

Uh oh!

markylaing commented Nov 17, 2025

Uh oh!

tomponline commented Nov 17, 2025

Uh oh!

kadinsayani left a comment

Uh oh!

roosterfish commented Dec 2, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

roosterfish commented Nov 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tomponline commented Nov 12, 2025

Uh oh!

roosterfish commented Nov 12, 2025

Uh oh!

tomponline commented Nov 12, 2025

Uh oh!

roosterfish commented Nov 12, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

tomponline commented Nov 12, 2025

Uh oh!

markylaing commented Nov 17, 2025

Uh oh!

tomponline commented Nov 17, 2025

Uh oh!

kadinsayani left a comment

Choose a reason for hiding this comment

Uh oh!

roosterfish commented Dec 2, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

roosterfish commented Nov 12, 2025 •

edited

Loading