Skip to content

Feature/etcd3 support with tests#308

Open
fbdlampayan wants to merge 4 commits into
skynetservices:masterfrom
nokia:feature/etcd3-support-with-tests
Open

Feature/etcd3 support with tests#308
fbdlampayan wants to merge 4 commits into
skynetservices:masterfrom
nokia:feature/etcd3-support-with-tests

Conversation

@fbdlampayan

@fbdlampayan fbdlampayan commented Nov 29, 2016

Copy link
Copy Markdown

etcd3-support feature tests.
fully utilizes the ones available in the server_test.go for etcdv2.
Also included are bug fixes for the ones caught by the testcases.

PS:
with this we would need etcd v3 running in Travis.

@miekg miekg left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Nice and thank you!

Bunch of comments.

Comment thread backends/etcd3/etcd3.go Outdated
"fmt"
"github.com/coreos/etcd/mvcc/mvccpb"
"encoding/json"
//"github.com/skynetservices/skydns/backends/etcd"

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

arrgh, yea my bad, missed deleting that one.

Comment thread backends/etcd3/etcd3.go Outdated
}

func (g *Backendv3) Records(name string, exact bool) ([]msg.Service, error) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

hmmm, whitespace is not but clutters this PR a bit.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

okay will remove the whitespaces a bit

Comment thread backends/etcd3/etcd3.go Outdated
if err != nil {
return nil, err
}
if resp == nil {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

why this? Isn't err != nil enough to bail out. Why not?

Comment thread backends/etcd3/etcd3.go Outdated
if w == "*" || w == "any"{
return true;
}
if keyParts[i] != w && (!(w == "*" || w == "any")) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I find this hard to parse, can we just add another if statement?

Comment thread backends/etcd3/etcd3.go Outdated
Nodes:
for _, item := range kv {

s := string(item.Key[:])

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Why the [:]

Comment thread backends/etcd3/etcd3errors.go Outdated
}

const (
KEYNOTFOUND = 100

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Can you add a comment where this value comes from?
Also, go nit, use CamelCase. And does it need to be exported?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

The value is arbitrary, so to respect on what skydns has been using with etcdv2 I used the value of 100.

Comment thread server/server.go Outdated
etcd "github.com/coreos/etcd/client"
"github.com/coreos/go-systemd/activation"
"github.com/miekg/dns"
etcd3 "github.com/skynetservices/skydns/backends/etcd3"

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

please sort this with the other skydns imports.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Ahhh noticed just now, it is sorted alphabetically. Okay will do

Comment thread server/server.go Outdated
// etcNameError return a NameError to the client if the error
// returned from etcd has ErrorCode == 100.
func isEtcdNameError(err error, s *server) bool {
func verdictEtcdV2(err error, s *server) bool {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

IsEtcd2NameError?

Comment thread server/server.go Outdated
return false
}

func verdictEtcdV3(err error, s *server) bool {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

isEtcd3NameError?

@fbdlampayan

Copy link
Copy Markdown
Author

updated with cleanup based on comments.

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