uber-go / fx

A dependency injection based application framework for Go.
https://uber-go.github.io/fx/
MIT License
5.68k stars 287 forks source link

config - Deep merge semantics? #213

Closed HelloGrayson closed 7 years ago

HelloGrayson commented 7 years ago

I don't see any docs or tests that describe how deep merge semantics work - are maps and objects merged, or is this just top-level?

https://github.com/uber-go/fx/tree/master/config

dmcaulay commented 7 years ago

I have noticed an issue during my development. Here's the the example yaml:

# config/base.yaml

modules:
  rpc:
    name: mormont
    tchannel:
      listenOn: localhost
      advertise: true
      peerList: /etc/uber/hyperbahn/hosts.json
    inbounds:
      - tchannel:
          port: ${UBER_PORT_TCHANNEL:0}
    outbounds:
      - service: rosetta
        http:
          host: localhost
          port: 14797
          path: /thrift
      - services:
          - populous
          - safari
          - treatment
          - trident
        tchannel:
          host: 127.0.0.1
          port: ${UBER_PORT_TCHANNEL:0}
# config/development.yaml

modules:
  rpc:
    name: mormont
    tchannel:
      listenOn: localhost
      advertise: false
    inbounds:
      - tchannel:
          port: ${UBER_PORT_TCHANNEL:0}
    outbounds:
      - service: rosetta
        http:
          host: localhost
          port: 14797
          path: /thrift
      - services:
          - populous
          - safari
          - treatment
          - trident
        tchannel:
          host: 127.0.0.1
          port: ${UBER_PORT_TCHANNEL:0}

The issue that I'm seeing is that advertise is still true when I run in development. Other keys are updated properly:

# config/base.yaml

uconfig:
  enabled: true
# config/development.yaml

uconfig:
  enabled: true
  UConfigDir: mormont-config/crc_configs

I haven't had time to dig in and determine the root cause.

ghost commented 7 years ago

Isn't advertise: false in the development.yaml?

glibsm commented 7 years ago

Thanks for reporting this, guys. I believe our intention is to deep merge (for yaml), otherwise having base.yaml becomes pointless really quick.

@alsamylkin it's false in development.yaml, but I think what Dan is saying is having loaded config, the key still returns true.

ghost commented 7 years ago

As for Grayson question, Merging works only for yaml provider and you should expect it to be the same as you'll Unmarshal two configs to the same variable.

We don't do merges between different providers, need to document it more clearly.

dmcaulay commented 7 years ago

The config issue that I was seeing was on my side. Thanks for the help @alsamylkin.

Do we still need to add docs?

ghost commented 7 years ago

Yep, created ticket to track GFM-340

dmcaulay commented 7 years ago

i'm not sure what the protocol is. should we close this and track in jira or keep both issues open?

ascandella commented 7 years ago

Moving to JIRA