Restructure Microcluster packages

[roosterfish]

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.

[tomponline]

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]

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]

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]

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.

[tomponline]

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.

[markylaing]

Few points from meeting:

1. 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.
2. Package moves all good :+1:
3. /internal/sql should not be on TLS listener (unix only).

[tomponline]

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

So important!

[roosterfish]

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.