Skip to content
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

Can't put module config in a profile #20

Open
WhittlesJr opened this issue Jan 30, 2019 · 28 comments
Open

Can't put module config in a profile #20

WhittlesJr opened this issue Jan 30, 2019 · 28 comments

Comments

@WhittlesJr
Copy link

I have some module config that varies between :duct.profile/dev and :duct.profile/prod. But the module is not initialized when it exists only in those profiles.

config.edn:

:duct.profile/dev
{:duct.module/foo {:bar :dev-value}}

:duct.profile/prod
{:duct.module/foo {:bar :prod-value}}

Now if I forget about profiles and just put the module config directly in the top-level of config.edn, the module works as expected.

Is this supposed to work and am I probably missing something?

@WhittlesJr
Copy link
Author

I'll add that :duct.module/foo does show up in the system as a function, but it does not apply that function to the system config.

@weavejester
Copy link
Contributor

weavejester commented Jan 30, 2019

Unfortunately this currently isn't possible without adding a fourth layer to how the system is processed.

Currently Duct turns a module configuration into a component configuration into a system:

modules -> components -> system

At the moment profiles are implemented as a type of module. Separating modules and profiles would require a fourth layer, e.g.

profiles -> modules -> components -> system

That's almost how 0.10 worked, except the profiles, modules and component layers were all mixed together in a haphazard way which caused a number of issues when the different pieces interacted.

Could you give a little more information as to what you're trying to do?

@WhittlesJr
Copy link
Author

OK, understood! So for configuration that differs between profiles, should I just be changing top-level keys instead of module options, like below?

config.edn

:duct.profile/dev
{:foo/bar :dev-value}

:duct.profile/prod
{:foo/bar :prod-value}

foo.clj:

(defmethod ig/init-key :duct.module/foo [_ _]
  #(duct/merge-configs
    %
    {::some-component (ig/ref :foo/bar)}))

I was essentially doing the following instead, using module options rather than top-level keys:

foo.clj (previously):

(defmethod ig/init-key :duct.module/foo [_ {:keys [bar] :as options}]
  #(duct/merge-configs
    %
    {::some-component bar}))

@weavejester
Copy link
Contributor

OK, understood! So for configuration that differs between profiles, should I just be changing top-level keys instead of module options

Right. To me that seems the simpler solution, since it doesn't mean separating profiles from modules. However, if it turns out to be particularly clunky, I'd be willing to think about other solutions.

I'd be very interested in some real-world examples, since it's hard to get a good feeling of what is most suitable when there's only small "foobar" code snippets.

@WhittlesJr
Copy link
Author

Got it, thank you!

My use case really is not any more sophisticated than the example I provided, but I sadly can't share any real code until I can convince my employer to open-source the project (some day maybe). Right now it's not really any better or worse to do it the suggested way, but it might be helpful to have some kind of disclaimer about not being able to configure modules via profiles in your documentation. (Unless you have that somewhere and I missed it?)

I did see that your web module has some kind of reference to the "environment" but I couldn't follow the logic... it's hard enough for me to keep the other two distinct notions of "profiles" straight in my head (leiningen profiles vs duct profiles).

@WhittlesJr
Copy link
Author

WhittlesJr commented Jan 30, 2019

Ah actually I lied, this does turn out to be slightly inconvenient, because my :foo/bar key has some simple logic attached to it (like a default value, or casting the value, or some such). Of course, you could do this with a (defmethod ig/init-key :foo/bar [_ v] (or v :default-value)), but it seemed convenient to just throw that into the module itself. So here's a closer example to what I'm doing:

(defmethod ig/init-key :duct.module/foo [_ {:keys [bar baz] :as options}]
  #(duct/merge-configs
    %
    {[:duct/const :foo/bar] (or bar :default-value)
     [:duct/const :foo/baz] (boolean baz)

     ::other-stuff-already-warranting-a-foo-module {}}))

So yeah I could just use init-keys for :foo/bar and :foo/baz, and it's all pretty trivial anyway. But my first instinct was to do it as above to keep it all in one place, and it was surprising that it didn't work.

@weavejester
Copy link
Contributor

Have you taken a look at ig/prep-key? That covers some of the cases that used to require a module.

@WhittlesJr
Copy link
Author

I had, and I just reviewed the information I could find on it. It sounds like, as you said, modules can sometimes be replaced entirely by profiles+prep-key? But I'm unclear on some things:

So, consider my module (:duct.module/foo) that adds some keys (my :foo/bar and :foo/baz) above. You could have a profile that adds those keys too (call it :duct.profile/foo). But say I want them to be added by including :duct.profile/foo and then customized in other profiles (:duct.profile/dev vs :duct.profile/prod). To my knowledge, there's no notion of profiles overriding each other in a hierarchy, right? So dev and prod can't just override foo predictably, can they?

Assuming that's not the approach you intended for profiles, that leaves me using a module (:duct.module/foo). It could add the keys as above. But to get the default-value (or similar) behavior above, I'd have to use prep-key, because I want dev and prod to be able to override the values (and as you said, I can't customize a module qua the module in profiles, I'd have to use top-level keys.)

So I'm left with the following, I think:

config.edn:

{:duct.profile/dev  {:foo/bar :dev-value}
 :duct.profile/prod {:foo/bar :prod-value-1
                     :foo/baz :prod-value-2}
 :duct.module/foo   {}
 }

foo.clj:

(defmethod ig/prep-key ::bar [_ v] (or v :default-value))
(defmethod ig/prep-key ::baz [_ v] (boolean v))

(defmethod ig/init-key :duct.module/foo [_ _]
  #(duct/merge-configs
    %
    {[:duct/const ::bar] nil
     [:duct/const ::baz] nil}))

Is that correct? It feels awkward and verbose. Is there a better way?

@weavejester
Copy link
Contributor

It's really hard to suggest a solution without knowing more about the problem. Is there any way you can talk about your use case in slightly more specific terms, without revealing any code? Using "foo" keys tells me roughly what you're trying to do, but not why.

@WhittlesJr
Copy link
Author

I really have been striving to convey the "why"... I'll try to boil it down to a list of needed features:

  • I need a central thing to include that adds keys to the system. (At one point I used :duct/include, now I use modules, it sounds like you can also use profiles)
  • The "central thing" is a modular feature that could be included in other configurations by itself
  • The added keys are referenced by other modular features
  • Those keys have some kind of behavior on them (defaulting / casting / etc.) but they must be in the system whether I explicitly customize them or not
  • The included thing must be able to have those keys overridden based on dev vs prod environment

Does that help?

@WhittlesJr
Copy link
Author

This seems like it would be a totally normative use case concerning how modules are to be developed... again, your web module seems to do some sort of magic to reference the dev vs. prod environment beyond the above stated mechanisms but I can't grasp it yet.

@weavejester
Copy link
Contributor

That helps a little, and I can at least give you a few options, though without further information I can't say which is the best solution for your problem.

Option 1

The first option is to do the same thing as Duct's middleware, which is to look for a key in the configuration called :duct.core/environment. This key is set to :production in production, and :development in development. So you could write:

(defmethod ig/init-key :foo.module/example [_ _]
  (fn [config]
    (if (= :development (:duct.core/environment config))
      ;; add dev setup to config
      ;; add prod setup to config
      )))

Option 2

If that's too coarse-grained, and you specifically need options in your config, then you can add a key that is just for passing per-profile options to the module:

(defmethod ig/init-key :foo.module/example [_ top-level-options]
  (fn [config]
    (let [profile-options (:foo.module.profile-options/example config)
          all-options     (duct.core/merge-configs top-level-options profile-options)
      ...)))

Option 3

The third option is not to use #duct/include at all, but instead manually merge the profiles. So in your dev.clj file, you override the read-config to:

(defn read-config []
  (duct/merge-config
   (duct/read-config (io/resource "todo/config.edn"))
   (duct/read-config (io/resource "dev"))))

Then remove these lines from your config:

 :duct.profile/dev   #duct/include "dev"
 :duct.profile/local #duct/include "local"

And instead have a dev.edn file that looks like:

{:duct.profile/local #duct/include "local"
 :duct.profile/dev
 {...}
 :foo.module/example {...}}

@weavejester
Copy link
Contributor

Additionally, I'll give this problem a little more thought and try to come up with a more general solution.

@WhittlesJr
Copy link
Author

Thank you! Here are my questions:

Option 1:

This seemingly moves the dev/prod decision-making to the module itself, rather than the configuration. So the module developer can build in dev/prod differentiation, but the module user can't? Unless I included a module option like so:

config.edn:

{:foo.module/example {:dev  {:option-a :dev-value}
                      :prod {:option-a :prod-value}}}

foo.clj:

(defmethod ig/init-key :foo.module/example [_ {:keys [dev prod] :as options}]
  (fn [config]
    (if (= :development (:duct.core/environment config))
      (duct.core/merge-configs config dev) ;; obviously it wouldn't be a straight merge but you get the point
      (duct.core/merge-configs config prod))))

Doable but pretty awkward, given that we already have a mechanism for differentiating dev vs prod config (the profiles).

Option 2:

As I consider it more, I think this is how you would address the concern above, right?

Option 3:

This seems like the most straightforward path, and I may end up going with it for now...

If what I originally assumed was going to work could be made to work (modules configured in a profile), that would be better than any of these workarounds. It was hard enough for me to grok the concept of how modules and profiles and duct itself all work. Once I got there though, I had such a nice, tidy mental model of the system, because duct is really elegant on its surface once you get a wide enough perspective of it. But then I hit this issue and my mental model had to suddenly become more complex and nuanced.

@WhittlesJr
Copy link
Author

Is it more than just resolving all profiles first, and then taking that config and resolving all non-profile modules next?

@WhittlesJr
Copy link
Author

Re-reading your earlier responses, I see that's what you said would be the needed solution. Is that difficult or error-prone, or might it have unwanted side-effects? Or is it a real option?

@weavejester
Copy link
Contributor

weavejester commented Jan 31, 2019

Let me see if I can explain the problem a little more.

In previous versions, it was possible for a module to reference another key. For example:

{:component/foo {:something 1}
 :module/bar    {:config #ig/ref :example/foo}}

In order for the module :module/bar to be initiated, we also have to initiate :component/foo. Allowing module keys and non-module keys to intermingle like this produced inconsistencies and complexies to the system.

To solve this, Duct 0.11 has a clear separation between modules and non-module keys. When you write a configuration in 0.11, all of the top level keys are modules. Behind the scenes, profiles are just modules that merge in their value into the configuration:

(defmethod ig/init-key :duct/profile [_ profile]
  #(duct/merge-configs % profile))

(It's a touch more complicated in practice, but not by much.)

So I want to maintain hygiene between modules and normal keys. I don't want to get into a situation where modules and non-module keys (which I'll call components) are allowed to reference each other. One solution is to add in a new profiles layer, so instead of writing:

{:duct.module/logging {}
 :duct.profile/base   {:duct.core/project-ns example}
 :duct.profile/dev    #duct/include "dev"}

We might instead write:

{:base {:duct.module/logging {}
        :duct.module/merge   {:duct.core/project-ns example}}
 :dev  #duct/include "dev"}

But I don't think that looks particularly elegant.

Another possible solution is to write modules like this:

(defmethod ig/init-key :duct/module [k opts]
  (fn [config]
    (let [opts (merge opts (config k))]
      ...)))

So a module looks for its own key in the configuration to get extra options. This means you could write:

{:duct.profile/dev
 {:example.module/foo {...}}
 :example.module/foo {}}

However, the unintuitive part is that the module needs to be at the top level as well as in the profile. I think this would be a big source of confusion.

So in a nutshell: we ideally want to put module configuration in profiles, but we also don't want to mix module keys with component keys. That's what makes the problem rather difficult to approach.

@WhittlesJr
Copy link
Author

OK, I can definitely see the difficulty. You know what though, I totally forgot about duct.core.merge/displace, and it seems to be sufficient for me in conjunction with top-level keys. I can get my dev/prod config differentiation the usual way (profiles) and my module will add in any keys not specifically customized. I feel pretty silly not even considering that until now.

I'm sure though that there are cases where this issue will cause a harder problem for the module developer, but so far I don't think I've actually run into any... At the very least, it's probably worth a mention in your documentation and flagged as a "gotcha." Like I said, it was intuitive for me to try it and surprising that it didn't work.

@weavejester
Copy link
Contributor

Thanks for the update. I'm glad you found an alternative solution.

I'll give this a little more thought and see if I can come up with something that feels more intuitive while still maintaining hygiene between modules and components.

@WhittlesJr
Copy link
Author

Thank you for being so attentive to my questions! I wish all open source maintainers were as on the ball as you are.

@WhittlesJr
Copy link
Author

I'm revisiting this issue because I'm now thinking about a use case where, depending on the profile, some modules may or may not be desired.

It's totally possible to just have a completely different config.edn for each separate case, I suppose. But my first thought was "wouldn't it be nice if I could do this specialization via profiles" since, like I've said before, profiles are an "obvious" mechanism to do such things from an outside perspective. Separate config.edns would also either cause duplication problems or involve some increased complexity by needing a merging strategy.

Another option I'm mulling over is to include all the modules anyway but, depending on an env variable, either add or don't add that module's keys. This also works, but it's a "code" solution whereas "data" solutions are of course preferred. (Although I already use a similar approach for having multiple "flavors" of each module, like "IP vs Serial" for a given communication protocol.)

And speaking of which, I'm not sure why I haven't shared this yet, since it's just a sketch of what my application does, but here's my concrete use case:

My application is an automated test system that targets embedded devices. My integrant system manages several stateful interfaces for communicating with these devices over various protocols, and I just use Midje to write the tests themselves that exercise these protocols. Up until now, I've only had one type of target device, which always needs certain interfaces (Modbus, BACnet, and an in-house UDP protocol, and each can have several "flavors" as above.) But now I'm targeting multiple different types of devices, which all require a different assortment of interfaces (and some new ones: CANopen and discrete/analog I/O.)

The necessary bits for each protocol (BACnet, Modbus, that UDP thing, CAN, discrete/analog) are currently collected into a separate module for each. As mentioned, the currently-configured "flavor" of each profile and other device-specific config (like IP address, COM port, etc.) are determined by env variables.

@WhittlesJr
Copy link
Author

WhittlesJr commented Mar 25, 2019

Whoops didn't actually include an actual question in there! The basic question is this:

Is there a good, data-driven way to turn these various features/modules/interfaces on and off, other than having separate config.edns?

@WhittlesJr WhittlesJr changed the title Can't seem to put module config in a profile? Can't put module config in a profile Mar 25, 2019
@weavejester
Copy link
Contributor

One option is to have a key in a profile that the modules look for to determine whether or not they should be on or off. Profiles will be merged before modules (because modules implicitly depend on the set of profiles), so modules can check any key set by a profile.

I'm going to think about this a little more, but it might take a while.

@WhittlesJr
Copy link
Author

I'm hoping we can come up with a nice, intuitive solution, but until then I'm getting by doing the kinds of workarounds we've been discussing.

@WhittlesJr
Copy link
Author

WhittlesJr commented Aug 12, 2020

I got bitten by this again. My module needs to add different keys to the system based on config that changes per profile, and I don't think that's possible with duct right now. (Please correct me if I'm wrong.) I find myself wishing that profiles and modules were two different layers, with profiles happening first. Here's the general pattern:

config.edn:

{:duct.profile/base {
  :duct.core/project-ns myproj}
 
:duct.profile/dev   #duct/include "dev"
 :duct.profile/local #duct/include "local"
 :duct.profile/prod  {}

 :duct.module/logging {}

 :myproj/module #ig/ref :myproj/config}

local.edn:

{:myproj/config [:a :b :c]
}

myproj.clj:

(defmethod ig/init-key ::module [_ mylist]
  (fn [system]
    (duct/merge-configs system
                        (reduce (fn [m k]
                                  (assoc m k {:config-goes :here}))
                                {}
                                mylist))))

This yields Missing definitions for refs: :myproj/config

@WhittlesJr
Copy link
Author

WhittlesJr commented Aug 12, 2020

Moving the :myproj/module config directly to local.edn (without the #ig/ref) allows the system to load, but :myproj/module doesn't actually get applied.

@weavejester
Copy link
Contributor

This design is the most promising fix to the module/profile problem. I haven't had time to implement it, but that's what I was planning on going with. I'd like to get your opinion on whether that would work well for you, and what you think of the syntax.

@WhittlesJr
Copy link
Author

That would be a huge step forward. Ideally you wouldn't need the :components and :modules submaps, but if it substantially reduces the complexity, then I certainly don't mind the extra syntax.

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

No branches or pull requests

2 participants