On not copy-pasting state and params struct conversions

[Dimiter Naydenov]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 10.02.2015 11:44, William Reade wrote:
> So, I think you *do* need conversion code specific to state -- the 
> methods exported from state should be in terms of the storage
> types, because they're the language we've chosen to model storage,
> so there needs to be conversion code in state (or possibly in an
> internal package -- but either way there's no reason to expose it
> to anyone else).
+1

I've added TODO comments to fields and structs in params and state
where we are using field types from external packages, like network.
We need to be *very* careful not to do that, because both params
request/response types and state docs should serialize the same way
and not change unexpectedly if the external package's type changes.
Otherwise we'll end up with hard to debug issues and incompatibilities
between juju versions.
> 
> WRT params, though, we already have two groups of client packages,
> so the same forces don't quite apply: and I still prefer that we
> *not* import other packages into params [0]. How about a single 
> params/convert package?
+1 Most of these conversions are straightforward and increase the
amount of boilerplate code. Using code generation scripts can help to
make the initial step of "making a params.Foo/state.Foo out of
somepackage.Foo" easier, but after that we need to check the code and
maintain it as-is. Please, always keep in mind the need for stability
of the serialized representations for things that go on the wire (API
request/response types and their fields) or in Mongo (doc types and
their fields).
> 
> Cheers William
> 
> [0] this is because the temptation to use a storage.Foo in place of
> a params.Foo has historically been overwhelming (and while it's
> *very dangerous* to do that it's hard to make the danger
> sufficiently obvious to dissuade distracted devs from doing so).
We can think about having tool to check we're not violating the
API/state type stability after we refactor the existing cases.

> 
> On Tue, Feb 10, 2015 at 11:07 AM, Andrew Wilkins 
> <andrew.wilkins at canonical.com> wrote:
>> Hi all,
>> 
>> Anastasia raised an issue in http://reviews.vapour.ws/r/885/
>> about how to cut down on struct conversions between params,
>> state, and domain packages. In this case we're talking about
>> storage. The following API server facades currently participate
>> in storage: - client - storage - provisioner - diskmanager (to be
>> renamed, this lists block devices on machines) - diskformatter -
>> storageworker (to be renamed, this is the dynamic storage
>> provisioner)
>> 
>> Each facade have some overlap in dealing with storage entities,
>> e.g. the diskformatter and diskmanager each need to deal with
>> block devices. This leads to much duplication of struct
>> copying/conversion code when toing and froing between state and
>> clients.
>> 
>> I don't want to go adding conversion code to the params, state or
>> storage packages, as they really shouldn't have dependencies on
>> each other. Does anyone have a good idea about where to put this
>> common functionality? Maybe "api/common/storage",
>> "apiserver/common/storage"? Does not appeal, but I can't think of
>> a better option.
I think as a rule of thumb, "to each its own" is useful here - e.g.
storage types need to be in storage package (or a subpackage thereof
to allow importing without loops), but things that go in state or over
the wire via the API need to be in subpackages of state and params.
>> 
>> Cheers, Andrew
>> 
>> -- Juju-dev mailing list Juju-dev at lists.ubuntu.com Modify
>> settings or unsubscribe at: 
>> https://lists.ubuntu.com/mailman/listinfo/juju-dev
>> 
> 


- -- 
Dimiter Naydenov <dimiter.naydenov at canonical.com>
Juju Core Sapphire team <http://juju.ubuntu.com>
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1

iQEcBAEBAgAGBQJU2d3ZAAoJENzxV2TbLzHwN1gIAK9eaSydcIhlmXGw1zue+mq1
86MeQjmQwB9l7C/Mqv/gK6MXfI7JUDB9cfgQ80ltyRLWhOI0rO2h/ICISDj3bEgp
F5SV7iZ8m9CdFhAuiS+waotzomojOiO1IZXQv5FCGCQ0yVAatYIezL/hH3jfRXS9
bRKJZDaaEYa+ORkPhu1rkAV9CDjbg4jS+T8tbkvZYcVkQjxRAPhITrMXL8uYzLep
Zzk8yU8+8tbq5lXbg99mNdQa0MSeU3gj6X1PzlTPycSTCxrr6Oviq1YaWD4lvfYQ
Mln27yW3hFbd8HAK6m7q4v5JS57auZO9H69f9rFvm33+TazUjShz5gOpZbQDf0A=
=+TQK
-----END PGP SIGNATURE-----