Dao Interface

[shawn-hurley]

Describe what this PR does and why we need it:
Having an interface for the DAO will allow us to switch persistent layers. This will also allow for unit testing in other parts of the code base.

Changes proposed in this pull request

• move etcd dao into dao/etcd package
• everything should now use the DAO interface

Does this PR depend on another PR (Use this to track when PRs should be merged)
depends-on
fix for PR #722 Work Item 1
Which issue this PR fixes (This will close that issue when PR gets merged)
This fixes #542

[eriknelson]

This has been on the agenda for a long time. Glad to see it finally.

EDIT: I coined the term DAO, but kind of hate it? Can anyone think of a better name for this? It's more of a Data Access Layer (DAL?) This PR would be the time to change it, if desired.

[maleck13]

I am a big fan of using an interface where there is a dependency. As I mentioned in the issue linked here, it will help with writing unit tests. So I am delighted that we are making this change.

Couple of personal preferences and thoughts

I think it would be good over time to split up this large interface into a set of smaller interfaces based around distinct capabilities. The reason for this is two fold

1. It allows for better encapsulation. Perhaps some areas only care about ServiceInstances and shouldn't know about ServiceBindings
2. Allows for simpler mocking. We will need to implement all of these methods in a mock to perhaps only enable a test that uses one of the methods

On another note. I think it is a good idea for the consumer package to define the interface rather than the provider. This can help avoid dependency issues (circular dependencies for example) as the consumer package does not directly depend on the provider package instead the consumer provides the interface it expects and this is fulfilled by the provider.

[shawn-hurley]

@maleck13 Sorry I am not fully following your recommendation regarding interfaces.

What I am seeing is dao is the consumer of dao/etcd(the provider) and therefore defines the interface.

Is that different then what you are referring to?

(edit: hit comment too quickly)

I think that an evaluation of this interface is a good idea as well. I would not mind breaking up dao into littler interfaces like such

type Dao interface {
    SpecDAO
    ...
}

type SpecDAO interface {
   GetSpec(string) (apb.Spec, error)
   ....
}

@eriknelson regarding the name. I can't think of anything that is significantly better the DAO. I also would love something better though 😄

[maleck13]

Sorry @shawn-hurley I will give a more concrete example of what I mean.

The broker package depends on the DAO. So in my suggestion the broker package would now define the interface that it needs as it knows its requirements best and so would no longer depend on
"github.com/openshift/ansible-service-broker/pkg/dao"
as the interface is defined within its own package. Does that make what I mean clearer. As mentioned this is only a suggestion

[shawn-hurley]

Ah ok, that does make more sense now thank you.

I would like to hear more about that helps prevent circular dependencies.
Also, is their an example in the standard library of using the interface like this? I am only asking because that might help me match use cases?

[jmrodri]

Is the idea we'll have multiples from here?

[shawn-hurley]

Yep, that was the initial plan/thought process

[jmrodri]

ACK

[jmrodri]

@eriknelson @shawn-hurley as far as names, the only thing that is descriptive is DAL since it is a data access layer vs DAO which usually means data access object.

[jmrodri]

@eriknelson @shawn-hurley correction I do have a name suggestion: AXON

http://www.dictionary.com/browse/axon

the long threadlike part of a nerve cell along which impulses are conducted from the cell body to other cells.

• unique
• means something
• cool
• NOT DAO

[jmrodri]

Are we holding off because of naming? or are we good?

[maleck13]

@shawn-hurley hope this is an interesting discussion ( I think so) but not my intention to hijack the PR :)
TLDR

• It is not a hard black and white rule.
• You could refactor a package consuming an interface to declare its own version easily if it became and issue
• It is really just a pattern I have found useful and to work well

Standard Lib:
I haven't gone hunting through the standard library to check, however i know the io package declares the Writer and Reader interface and consumes them heavily throughout that package.
So in my opinion, the io package knows what it wants in order to consume a reader or a writer, so it is the package that declares the interfaces. However in this case, it is an exported interface as it is generic enough that it can be useful outside of the io package. If it was not useful outside of the io package I don't think it would be exported and would just be an internal interface. There are some of these in the io package also.

On the circular dependencies: (this really is less of a thing but a side benefit)
By declaring the interface within the consuming package you reduce the the chance of a circular dependency as you have removed an import of an external package.