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

Fixes #2089 - Add network configuration to ENC #1862

Closed
wants to merge 1 commit into from

Conversation

ares
Copy link
Member

@ares ares commented Oct 17, 2014

No description provided.

"subnets"=>
[{"network"=>"2.3.4.0",
"name"=>"one",
"gateway"=>nil,
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure how well YAML (or at least the Puppet ENC) handles nil values for keys - should we consider not including nils in the ENC YAML?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure I follow. nil should never become key, subnets is always array of hashes and some values may be nil which becomes undef (well I tested only on one puppet version)

I'd say that it would be more convenient to always have gateway key which sometimes has address as a value and sometimes is undef. Some subnets does not have gateway which is valid value.

Copy link
Member

Choose a reason for hiding this comment

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

To quote myself: "nil values for keys" - it's the values that are nil, not the keys. If they get correctly converted to undef then thats fine - we've strugged to pass undef in various places in the past, which is why I'm raising it

Copy link
Member Author

Choose a reason for hiding this comment

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

It's "for keys" part that I didn't understand. Anyway if you get to same results I think undef is the best fit here.

@ares
Copy link
Member Author

ares commented Oct 21, 2014

Do we want to convert Nic::Managed to Interface and Nic::Bond to Bond in interface type?
edit:
we do (not translated ofc), I'll reuse mapping that will be used in API

@ares
Copy link
Member Author

ares commented Oct 23, 2014

@GregSutcliffe your comment was removed (EDIT: and is back again), but here's my answer - it works in development which tends to be issue with STI but since we have require_dependency for children in all NIC classes it should work fine

@ares ares force-pushed the feature/2089-networking-enc branch from 66bed40 to 8e5f029 Compare October 24, 2014 07:20
@GregSutcliffe
Copy link
Member

@ares I don't think this loading @host.subnet correctly for the built-in interface:

  interfaces:
  - mac: 52:54:00:e0:bf:95
    ip: 192.168.123.2
    type: Interface
    name: 
    attrs: {}
    virtual: false
    link: true
    identifier: 
    managed: true
    subnet: 

The host definitely has a subnet:

[3] pry(main)> Host.find(745).subnet
  Host::Managed Load (0.8ms)  SELECT "hosts".* FROM "hosts" WHERE "hosts"."type" IN ('Host::Managed') AND "hosts"."id" = $1 LIMIT 1  [["id", 745]]
  Subnet Load (0.9ms)  SELECT "subnets".* FROM "subnets" WHERE "subnets"."id" = 6 ORDER BY vlanid LIMIT 1
=> #<Subnet id: 6, network: "192.168.123.0", mask: "255.255.255.0", priority: nil, name: "Static", vlanid: "", created_at: "2014-01-15 15:51:03", updated_at: "2014-01-15 15:51:03", dhcp_id: nil, tftp_id: nil, gateway: "192.168.123.1", dns_primary: "192.168.123.1", dns_secondary: "172.20.10.1", from: "", to: "", dns_id: nil, boot_mode: "Static", ipam: "DHCP">

@ares ares force-pushed the feature/2089-networking-enc branch 2 times, most recently from fd5eff2 to f61a165 Compare October 24, 2014 14:29
@ares ares force-pushed the feature/2089-networking-enc branch from f61a165 to 568ba10 Compare October 24, 2014 15:11
@GregSutcliffe
Copy link
Member

Reviewed, tested, looks great for my various networking test hosts. Merged as a1b2ee5, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants