-
Notifications
You must be signed in to change notification settings - Fork 21
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
:duct/profile merge order is unintuitive #31
Comments
Profiles are a type of module, and therefore probably should inherit from
The difficulty here is that modules and components also need to be separated, as their references are incompatible. So if we want to separate profiles from modules, we end up with a configuration three layers deep: {:duct.profile/base
{:duct.module/logging {}
:duct.module/sql {}
:duct.module/merge
{:custom.service/echo
{:log #ig/ref :duct/logger
:db #ig/ref :duct.database/sql}}}} However, we could implement modules as metadata on the configuration: {:duct.profile/base
^:duct.module/logging
^:duct.module/sql
{:custom.service/echo
{:log #ig/ref :duct/logger
:db #ig/ref :duct.database/sql}}} There are a few advantages to this approach:
The disadvantage is obviously that we'd break backward compatibility. |
I'm not sure I follow all the reasoning of your proposal. Firstly:
Yes I understand that, though this feels like a hack to me; as I don’t think profiles are like modules at all. I could be wrong on this, but it seems like you’re making them modules primarily for the perceived benefit of having as few core concepts as possible in duct. I definitely appreciate the desire to do this (we all love Scheme right? 😄), but I don’t think it pans out in practice. It's like you make them the same as modules only to then artificially force them to be handled differently (i.e. their evaluation happens at a different time, and profiles deactivate all their refs. Additionally I find the idea of managing profile merge order through integrants topoligical sort to be confusing. I think I'd be more inclined to think of profiles as modules if duct itself didn't need to know about them, i.e. I think their presence in duct code like This said I don't in principle have a problem with duct having two concepts profiles and modules :-)
I understand the references between modules and (system) components are incompatible; however I don't think you need to reflect the differences between profiles and modules in the config... i.e. can't profiles just be different from modules, handled specially by duct (they are already). Essentially have I should also state I don't quite understand what
I'm really not a fan of this approach, as it hides modules and makes them more magic. I think also nested metadata definitions may get confusing e.g. having a |
It's partially that, but it's also partially that profiles are modules by definition. A module is a pure function that transforms the configuration. A profile is also a pure function that transforms the configuration, just in a more specialised way.
I want to avoid placing two incompatible things in the same map, and ideally I'd like to ensure that modules can be specified per profile. One of the weaknesses of the current design is that you can't have per-profile module configurations.
It's a module that merges its content into the configuration. In other words, it's what profiles are now. If we're going to be going down the route of profiles → modules → components, then we need a module that merges in components into the configuration.
How does it hide them? They're visible in the edn configuration. I guess it hides them if you run
You can add metadata to a module's configuration. I don't think this hugely impacts readability, and in any case, it shouldn't be common use. {:duct.profile/base
^{:my.module/foo ^:displace {:bar 1}}
{:custom.service/baz {}} I'm not sure why you'd want to displace or replace an entire map of modules. It sounds like something we'd want to prevent in general, as it leads to uninitutive results. My inclination is that profiles should also not have top-level merge metadata, as that could implicitly change the merge order of profiles. |
Metadata pros
Metadata cons
|
I’m still not 100% sure I’m clear on the semantics of your proposal… How does it let one specify the priority ordering of profiles? I think the main thing for me is that the semantics of profiles should be an explicit and very clear (and customisable) precedence order. I think also that an expansion from profiles → modules → components, makes sense… Though I’d like to clarify that I think profiles must also be able to contain components too. I strongly feel those profiles should be merged in a specifiable order and explicit order. And there is non clearer to me than providing: Whilst I’m less concerned at this stage with the syntax used and am more interested in ensuring profiles can merge in an explicit order, the proposed metadata syntax will I think be very, very confusing. For example using an ataraxy module:
So now we’re doing a metamerge over meta-data, which will be more awkward to inspect and won’t pprint by default. Also a more minor objection but more metadata will make it more awkward to get the syntax right; as it interferes with the natural :k :v :k :v rhythm of map syntax. It also moves further away from the integrant foundations… and makes duct more bespoke syntactically than integrant. |
Thanks for getting back to me about this. Your feedback is really useful, and though I'm arguing for the metadata syntax, I'm currently undecided.
The
I'd like to get away from modules that have significant amounts of configuration, for reasons I'll explain. The Araraxy module has been already removed from the Duct project template in favour of the Ataraxy router, as the introduction of Modules currently have their own options, but they can also read the configuration and make decisions based on the information they receive there. Part of the difficulty I've had with modules is figuring out what information to put in a module's options, and what information to store in the configuration instead. For example, you can supply the database URL to the Duct SQL module directly: {:duct.module/sql {:database-url "postgres://localhost/test"}
:duct.profile/base {}} Or you can specify it by overriding the {:duct.module/sql {}
:duct.profile/base
{:duct.database/sql {:connection-uri "postgres://localhost/test"}}} My inclination is to favour the latter example. It's not significantly more verbose, and it makes explicit what the module is doing behind the scenes. As I've developed Duct, my experience is that modules rarely need much, or indeed any configuration. It usually makes more sense for them to look at the current configuration to determine what to do. So the fact that metadata makes it hard to make complex configurations doesn't seem like a significant disadvantage to me, as Duct has been moving away from that for a while. That said, I would be interested to hear about any modules that people have build that do require significant configuration.
Let's take a quick look at the different options for a default configuration. This is the configuration currently: {:duct.module/logging {}
:duct.module/cljs {:main foo.client}
:duct.module.web/site {}
:duct.module/sql {}
:duct.profile/base
{:duct.core/project-ns foo
:duct.router/ataraxy
{:routes {[:get "/example"] [:foo.handler/example]}}
:foo.handler/example
{:db #ig/ref :duct.database/sql}}
:duct.profile/dev #duct/include "dev"
:duct.profile/local #duct/include "local"
:duct.profile/prod {}} This is how it would look using metadata: {:duct.profile/base
^:duct.module/logging
^{:duct.module/cljs {:main foo.client}}
^:duct.module.web/site
^:duct.module/sql
{:duct.core/project-ns foo
:duct.router/ataraxy
{:routes {[:get "/example"] [:foo.handler/example]}}
:foo.handler/example
{:db #ig/ref :duct.database/sql}}
:duct.profile/dev #duct/include "dev"
:duct.profile/local #duct/include "local"
:duct.profile/prod {}} Realistically, I'd probably add in a default for {:duct.profile/base
^:duct.module/logging
^:duct.module/cljs
^:duct.module.web/site
^:duct.module/sql
{:duct.core/project-ns foo
:duct.router/ataraxy
{:routes {[:get "/example"] [:foo.handler/example]}}
:foo.handler/example
{:db #ig/ref :duct.database/sql}}
:duct.profile/dev #duct/include "dev"
:duct.profile/local #duct/include "local"
:duct.profile/prod {}} Finally, this is how it would look like with a nested configuration: {:duct.profile/base
{:duct.module/logging {}
:duct.module/cljs {:main foo.client}
:duct.module.web/site {}
:duct.module/sql {}
:duct.module/base
{:duct.core/project-ns foo
:duct.router/ataraxy
{:routes {[:get "/example"] [:foo.handler/example]}}
:foo.handler/example
{:db #ig/ref :duct.database/sql}}}
:duct.profile/dev #duct/include "dev"
:duct.profile/local #duct/include "local"
:duct.profile/prod {}} My thought is that the ;; ensure all other modules come after :duct.module/base
(defmethod ig/prep-key :duct/module [_ options]
(assoc options ::depends (ig/ref :duct.module/base)))
(defmethod ig/init-key :duct.module/base [_ config]
(constantly config)) Which configuration style do you think seems the most accessible? |
Great 👍 👍
👍 Generally I prefer this approach; we moved to the router syntax in ataraxy a long time back; because of the limitations of the easier syntax.
Ok that's interesting to hear. Whilst I agree with you that we rarely put any config in modules, I think I also have a use-case coming up for a module in our app (not as a library) which will need a relatively significant piece of config. It's really a data-macro scenario, where our app has multiple customer configs in it, and customers may have wildly different navigation, with different routes etc. I'd like to essentially move away from having to configure handlers etc, and move to configuring features in the navigation tree. I mention this only to say I think modules, as much as I'm wary of them, are also a very useful feature to support this kind of thing. Anyway it's interesting that modules will have less config associated with them, that certainly counters some of my critique. Though I still think it's weird how much power will be hidden not just behind implicit mechanisms like I definitely prefer the more explicit non meta-data nested design. One question, is Thanks again for everything. |
Yes, that would be a requirement. I tried having modules and components in the same configuration, and it lead to too many issues, particularly around resolving references. I definitely want to keep them separate. An alternative syntax is to separate them by key: {:duct.profile/base
{:modules
{:duct.module/logging {}
:duct.module/cljs {:main foo.client}
:duct.module.web/site {}
:duct.module/sql {}}
:components
{:duct.core/project-ns foo
:duct.router/ataraxy
{:routes {[:get "/example"] [:foo.handler/example]}}
:foo.handler/example
{:db #ig/ref :duct.database/sql}}}
:duct.profile/dev #duct/include "dev"
:duct.profile/local #duct/include "local"
:duct.profile/prod {}} |
This is certainly a concern of mine. I think there's a balancing act between making configuration explicit but manually written, and implicit but automatically generated. One thing I'm curious about; why do you say the metadata is "hidden"? Surely it's only hidden if you're looking at the |
I think I too would rather use the non-meta data structure for modules. I'm neither familiar with Java's annotations nor Python's decorators so I can't relate to that point you made. If we see backwards compatibility much of an issue then maybe we could introduce a versioning key in Also since we are talking about modules and components, I'd like to point out that the issue #29 potentially relates to this. |
Yes, I do We've drifted somewhat from the standard duct project template, because we run a multi-tennant configuration; where each customer has their own set of profiles that get resolved by some wrappers over the duct functions, e.g. in dev we'd do |
The merge order of
:duct/profile
's does not work as expected.Firstly one might reasonably expect that the merge order of profiles is determined by the order of profiles listed in the
profiles
argument to functions such asduct.core/exec-config
, e.g. a user might expect the call:to have the profile
:my-profile/a-two
meta-merge
over the profile:my-profile/b-two
. However the above profile will actually always be merged in the order of[:my-profile/a-two :my-profile/b-one]
i.e. alphabetically on key name, due to the fallback comparator inside integrants key-comparator.As duct profiles are almost modules one might try to force an ordering across them by establishing a dependency chain e.g:
However, this doesn't work because any key deriving from
:duct/profile
has any#ig/refs
they contain deactivated and converted into InertRefs here. Which means the subsequent call to fold-modules won't apply the profiles in the topological dependency order.I should state that even if it worked I find the later solution significantly more confusing to reason about than an explicit ordering based on the order of the given profile keys. In particular in the case where you have multiple profile chains (with some shared and potentially optional profiles at various points in the chain e.g.
[:duct.profile/base :project.profile/customer :duct.profile/dev :project.profile/customer-dev :project.profile/local]
[:duct.profile/base :project.profile/customer :duct.profile/test]
[:duct.profile/base :project.profile/customer :project.profile/customer-prod :project.profile/local]
This is really easy to reason about if each of the profile chains are specified like so; however when they're declared through dependencies they become a graph, and the intent is somewhat lost. Also I tend to think of profiles as maps that get merged with a well defined precedence to form a complete system; and don't think profiles depend on each other; they're just merged into a complete artifact.
On slack @weavejester mentioned there were 3 designs for modules
I think the implementation is currently actually different from the intended design here, and is much more like the second suggested design. i.e. profiles and modules exist as separate things but at the same layer, e.g.
(isa? :duct/profile :duct/module) ;; => false
.I'd certainly vote for a move towards profiles containing modules and normal configuration; with them being merged together prior to the system initiation in a well defined explicit and provided order. The risk with this move is that it might break peoples applications if they have defined custom profiles; however those users will currently be getting profiles merged in an alphabetical order. So perhaps we could effectively deprecate the current
duct.core/exec-config
but leave that merging profiles in alphabetical order, and create a new functionduct.core/execute-config
that applies profiles in the specified order?An additional complication is what the semantics of keyword inheritance for profiles should be? e.g. we could possibly specify things like this:
However when merging the profiles/refset for
:duct.profile/prod
the above the order will have to fallback to alphabetical between those profiles. Personally I think this should be strongly discouraged; as accidental orderings can easily arise, and that profiles should only use explicit keys.The text was updated successfully, but these errors were encountered: