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

Do a deep merge on bindings rather than a shallow #419

Merged
merged 1 commit into from
Feb 27, 2019

Conversation

n1koo
Copy link
Contributor

@n1koo n1koo commented Feb 19, 2019

What are you trying to accomplish with this PR?
Make it possible to have multiple overlapping configurations that will be deep merged to form one sane configuration.

Eg. lets say you have following configurations:

common.yaml:

common:
  registry: private-thingy
...
app:
  port: 4200
  name: cat-service
  lb_id: nonprod
....

and then you have environment specific one:

production.yaml:

app:
  lb_id: prod

Currently we only merge top level entries with merge so eg. if lb_id was on top this would be fine but for more complex configurations people tend to separate things in to nested entries. Another example could be eg. setting resources but overriding limits in certain env, think of:

  resources:
    limits:
      cpu: "3"
      memory: "1Gi"
    requests:
      cpu: "3"
      memory: "1Gi"

and then in nonprod you wanna override the requests for example.

How is this accomplished?

  • Pull the bindings loop in to the parser
  • do a deep_merge! instead of merge!
  • profit

tl;dr s/merge/deep_merge but I thought its cleaner to pull the loop in to the parser too. Makes testing lot more easier and harder to screw up if doing changes in both deployer and renderer

What could go wrong?
Things dont get parsed, world blows up.

Seriously though, if someone is relying on the wrong behaviour (shallow merge) this might cause a surprise, but technically doing so is wrong .. so :)

Copy link
Contributor

@dturn dturn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please rebase so we can get the tests ✅ before merge.

@n1koo
Copy link
Contributor Author

n1koo commented Feb 22, 2019

Rebase done

@@ -7,7 +7,15 @@ module KubernetesDeploy
module BindingsParser
extend self

def parse(string)
def parse(binds)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this PR is 👍 functionality-wise, but I feel like this class's API is a bit weird now--it's not intuitive what the difference is between BindingsParser::parse and BindingsParser::parse_binding would be.

Here's a suggestion:

# If you need multiple, you do:
bindings = KubernetesDeploy::BindingsParser.new
# ...
  opts.on("--bindings=BINDINGS", "...") { |b| bindings.add(b) }
# ...
bindings.parse

# But you can still also use the exact same API as before:
KubernetesDeploy::BindingsParser.parse(bindings_string)

module KubernetesDeploy
  class BindingsParser
    def self.parse(string)
      new(string).parse
    end

    def initialize(initial_string=nil)
      @raw_bindings = Array(initial_string)
    end

    def add(string)
      @raw_bindings << string
    end

    def parse
      result = {}
      @raw_bindings.each do |string|
        # parse/merge
      end
      # etc
      result
    end

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense, refactored

@n1koo n1koo force-pushed the deep_merge_on_bindings branch 2 times, most recently from 983ee09 to f6daf34 Compare February 26, 2019 07:09
Copy link
Contributor

@KnVerey KnVerey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ready to ship after typo fix and tophat

@@ -38,6 +37,7 @@ ARGV.options do |opts|
exit
end
opts.parse!
bindings = bindings.parse
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be parser.parse, which reminds me that someone needs to tophat all affected executables before we merge this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I did my tophat with render 🤦‍♂️

@KnVerey
Copy link
Contributor

KnVerey commented Feb 27, 2019

I was able to confirm successful 🎩 using the collection-with-erb fixtures with both kubernetes-render and kubernetes-deploy. We'll :shipit: once CI passes.

@dturn dturn merged commit 89ef125 into Shopify:master Feb 27, 2019
@timothysmith0609 timothysmith0609 temporarily deployed to rubygems March 27, 2019 16:53 Inactive
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

Successfully merging this pull request may close these issues.

4 participants