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

Implement interface specification syntax #58

Merged
merged 12 commits into from
Jan 5, 2016

Conversation

justenwalker
Copy link
Contributor

For #53

Changes

  • Factor out IP/Interface logic into its own go file
  • Implement stable sorting of interfaces and their IP
  • Implement Interface Specification syntax for the interfaces option

Interface Specification

  • eth0 : Match the first IPv4 address on eth0 (alias for eth0:inet)
  • eth0:inet6 : Match the first IPv6 address on eth0
  • eth0[1] : Match the 2nd IP address on eth0
  • 10.0.0.0/16 : Match the first IP that is contained within the IP Network
  • fdc6:238c:c4bc::/48 : Match the first IP that is contained within the IPv6 Network
  • inet : Match the first IPv4 Address (excluding 127.0.0.0/8)
  • inet6 : Match the first IPv6 Address (excluding ::1/128)

Stable Sorting

Interfaces and their IP addresses are ordered alphabetically by interface name, then by IP address (lexicographically by bytes).

  • eth0 10.2.0.1 192.168.1.100
  • eth1 10.0.0.100 10.0.0.200
  • eth2 10.1.0.200 fdc6:238c:c4bc::1
  • lo ::1 127.0.0.1

}
}

func TestOnChangeCmd(t *testing.T) {
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 TestOnChangeCmd and TestHealthCheck got moved to this file accidentally.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved it ( 0e0a934, 6eca67f)

@tgross
Copy link
Contributor

tgross commented Dec 30, 2015

We should add cover/coverage.out to the .PHONY block of the Makefile. I got confused trying to check something in the tests locally because I didn't have a clean test environment.

* state. */
if interfaceIPsErr != nil && len(interfaceIPs) > 0 {
log.Printf("We had a problem reading information about some network "+
"interfaces. If everything works, it is safe to ignore this"+
Copy link
Contributor

Choose a reason for hiding this comment

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

incredibly tiny nitpick: you'll want a space here after "this"

@justenwalker
Copy link
Contributor Author

@tgross Found a better way to handle interface spec matching using golang interfaces. Refactored the code accordingly (9b3cd7a)

// Interface Spec
type interfaceSpec interface {
Match(index int, iip interfaceIP) bool
String() string
Copy link
Contributor

Choose a reason for hiding this comment

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

As far as I can tell this is an unused method.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Isn't String() used implicitly when doing fmt.Printf ? https://golang.org/pkg/fmt/

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, but we only care about that if the fmt is user-visible, in which case we could just pass someSpecInstance.Spec to the formatter.

@justenwalker
Copy link
Contributor Author

@tgross Most of the new comments relate to the String() method (and inheriting it through baseInterfaceSpec). I am still new to go - so I'm probably coding Java in golang instead of doing something more idiomatic. If you want me to cut out String() completely, let me know.

- Remove String() from interfaceSpec
- Put Spec in each implementation struct
- parseInterfaceSpec can now return nil on error
@justenwalker
Copy link
Contributor Author

@tgross I've removed String() and baseInterfaceSpec in 3379712. Since I'm using an interface type instead of a struct type, it can return nil instead of needing some kind of base class. I think this is for the best.

@tgross
Copy link
Contributor

tgross commented Jan 5, 2016

Most of the new comments relate to the String() method (and inheriting it through baseInterfaceSpec). I am still new to go - so I'm probably coding Java in golang instead of doing something more idiomatic.

Honestly, if this were a non-technical-user-facing application, I'd say it'd be the right way to go because you're going to want to return pretty-printed errors to the user.

But for Containerbuddy's audience, returning the default struct formatting by default is probably a better approach when it comes to debugging and logging -- this way the user gets whatever fields in the struct that we've managed to construct at that point instead of just the Spec field, which is (in a sense) just the input to the constructor.

@tgross
Copy link
Contributor

tgross commented Jan 5, 2016

Ok, this is good-to-go. Nice work, @justenwalker

tgross added a commit that referenced this pull request Jan 5, 2016
Implement interface specification syntax
@tgross tgross merged commit 9428053 into TritonDataCenter:master Jan 5, 2016
@justenwalker justenwalker deleted the interface_spec branch January 5, 2016 14:56
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.

2 participants