redesign api with more align to libvirt api about go-libvirt HOT 20 CLOSED

Hey @vtolstov,

I agree, as the project continues to grow we are more and more in need of some API organization. Modifying the existing API to take a Domain is one option, but I think we'll soon find ourselves in the same situation.

I think a better alternative may be to group operations on domains, volumes, etc, within their respective type e.g., Domain.Migrate() or StoragePool.Refresh().

This is a fairly large change and while I know you've put considerable effort into #31, it may not be the place. PRs should encompass small changesets under a single topic/feature, i.e., Adds ability to create domain from XML.

from go-libvirt.

I think a better alternative may be to group operations on domains, volumes, etc, within their respective type e.g., Domain.Migrate() or StoragePool.Refresh().

Yes i think about it, what do you prefer - create util package that have this abstraction above plain libvirt api, or modify current package?

This is a fairly large change and while I know you've put considerable effort into #31, it may not be the place. PRs should encompass small changesets under a single topic/feature, i.e., Adds ability to create domain from XML.

I can rewrite all code and create one pr with all that changes (also i'm plan to split big libvirt.go file to logical parts (storagepool.go, network.go, secrets.go, storagevolume.go and so)

from go-libvirt.

If that possible you can add my account to write access(i'm not plan to push to master, but develop inside separate branch)

from go-libvirt.

@benlemasurier i'm complete rewriting all the stuff, please check https://godoc.org/github.com/vtolstov/go-libvirt what do you think?

from go-libvirt.

@benlemasurier i'm complete rewriting all the stuff, please check https://godoc.org/github.com/vtolstov/go-libvirt what do you think?

This is looking good. If you intend to submit a PR, I'd like to see a 1:1 mapping of existing functionality. In its current state we have a lot of new features such as Create, Reboot, etc.. Once we lock in the new API I'll be happy to work in new features.

If that possible you can add my account to write access(i'm not plan to push to master, but develop inside separate branch)

Working from your own branch/account will work well for now.

from go-libvirt.

I like the API but I have a slight concern that embedding the libvirt connection everywhere will result in misuse of it with long lived domains and such.

I had something like l.Domains.Create(dom) in mind instead of dom.Create(). This way, the connection has to be explicitly passed around, and the structures are just containers for data.

See go-github for an example of this pattern.

Thoughts?

from go-libvirt.

I had something like l.Domains.Create(dom) in mind instead of dom.Create(). This way, the connection has to be explicitly passed around, and the structures are just containers for data.

sounds like a better approach to me.

from go-libvirt.

I like the API but I have a slight concern that embedding the libvirt connection everywhere will result in misuse of it with long lived domains and such.

api inspired by libvirt-go and python and ruby bindings. i don't understand what is misuse ?

from go-libvirt.

This is looking good. If you intend to submit a PR, I'd like to see a 1:1 mapping of existing functionality. In its current state we have a lot of new features such as Create, Reboot, etc.. Once we lock in the new API I'll be happy to work in new features.

may be after all rewriting stuff...

from go-libvirt.

I had something like l.Domains.Create(dom) in mind instead of dom.Create(). This way, the connection has to be explicitly passed around, and the structures are just containers for data.

why dom.Create? l.DomainCreateXML and l does not passed, but Domain - this is container for all needed methods. I don't think that api like d.Shutdown(l) is good design.

from go-libvirt.

The point I'm trying to make is that I think the libvirt connection should be used explicitly, not implicitly by types which should be data containers.

Example:

type DomainService struct {
    c *libvirt.Conn
}

func (s *DomainService) Create(d *Domain) error {
    // Create a domain using s.c.
}

And then it would be called as:

c := libvirt.New()
d := &Domain{Name: "Foo"}
if err := c.Domains.Create(d); err != nil { panic(err) }

This way, it is quite obvious that the libvirt connection is acting on the domain. The domain is just a container for data.

I understand the pattern you're proposing, but I haven't seen it as commonly used in Go as in other languages. My counterpoint is that it seems much more obvious that the libvirt connection is being manipulated in some way when it's explicitly called to act upon some container.

from go-libvirt.

See: https://godoc.org/github.com/google/go-github/github.

You'll notice that the individual types do not embed a Client: they are just containers for data: https://godoc.org/github.com/google/go-github/github#Notification.

from go-libvirt.

c := libvirt.New()
d := &Domain{Name: "Foo"}
if err := c.Domains.Create(d); err != nil { panic(err) }

i think that its overhead instead of using two lines to create connection and domain you need 3 lines... what benefits of c.Domains.Create(d) ?

from go-libvirt.

In Go, we prefer explicitness and readability over attempting to remove one line of code.

The example I posted explicitly uses the libvirt connection to act upon the domain passed as a parameter.

In addition, with your method, it isn't possible to create a domain because the libvirt connection field is unexported. Exporting it is asking for trouble.

d := &Domain{Name: "foo"}
d.Create() // panic!

from go-libvirt.

I am AFK for most of the day and may be slow to reply. @benlemasurier please do share your thoughts as well.

from go-libvirt.

from go-libvirt.

Hello
I'm not familiar with the specifics of this page, but I'd just like to point out that functional options are a nice way of organizing a friendly API :

• the signatures stay the same, even when you change possible options,
• no need to define param structs,
• caller is not cluttered with options she doesn't need/want to deal with
• works fine with complex options behavior needs.

See https://dave.cheney.net/2014/10/17/functional-options-for-friendly-apis

The sample constructor looks like

func NewServer(addr string, options ...func(*Server)) (*Server, error)

from go-libvirt.

d := &Domain{Name: "foo"}
d.Create() // panic!

simplify - if l inside container not init - return error.
In you case what happening when domain not defined? You have error to, so in my case and you case - we need to check domain existence on libvirt host.

from go-libvirt.

c := libvirt.New()
d := &Domain{Name: "Foo"}
if err := c.Domains.Create(d); err != nil { panic(err) }

This is again brings two steps - firstly we need to check via lookup domain using libvirt connection, and when - start it if it exists.
But when domain holds already libvirt link - we already know that domain exists. And when we need additional steps we already know that domain exists and don't need additional lookup.

How DomainService helps in this case?

from go-libvirt.

I'm going to close this, now that the API is exactly the libvirt API.

from go-libvirt.