skilld-labs / go-odoo

Golang wrapper for Odoo API
Apache License 2.0
86 stars 62 forks source link

Add support for /odoo/service #11

Closed blaggacao closed 4 years ago

blaggacao commented 6 years ago

... especially the db service would make this api bindings quite complete for a terraform provider...

blaggacao commented 6 years ago

trying to address this: https://github.com/xoes/go-odoo

blaggacao commented 6 years ago

On that note, I see some lobying must be done in order for Odoo to accept a full CRUD compliance with their database lifecycle management. C + D are already in place (yet not returning really anything useful, like for example DB UUID), but R + U (ok what's the scope of them?) are inexistent at the db service level.

This is bad and makes odoo istances very ugly to script. I have decided just to do some patches and try to pull them upstream because it's easier in the long run than a lot of bad and hard to maintain boilerplate.

That being said, I'd like to encourage to divide this go library in effectively two separate ones:

ahuret commented 6 years ago

Hi @blaggacao you could take a look on what I did on https://github.com/antony360/go-odoo (master and on PR on skilld-labs). I replaced python and csv files by golang templates and a cobra command line tool to generate models and let client choose which models he wants, using its own odoo instance to generate them. (possibility to use custom models).

blaggacao commented 6 years ago

It looks very promising: Here my comments as they are hard to find in the huge diff:

Otherwise I really love the idea with a api cli. This is tremendously powerful! I also agree in removing the report hook, it's so easy to create it. If it should not be lost for good, it could be made an example in the readme...

A last one spotted: (maybe in another commit or PR): I found out that the true datastructure of the api endpoint is rather two step (Config + Session). This cleanly opens the door for accessing the other services exposed by Odoo cleanly. Here is what I use in my code:

From: https://github.com/xoes/go-odoo/blob/master/client/client.go

type Config struct {
    HostURL         string
    AdminPassword   string  # Important for some other exposed services
    Transport       http.RoundTripper
    *Session
}

type Session struct {
    DbName   string
    Username string
    Password string
    UserID   int
}

type CreateDatabaseConfig struct {
    AdminPassword string
    DbName string
    Demo bool
    Lang string
    UserPassword string
    Login string
    CountryCode string
}

The CreateDatabaseConfig struct is already an adaption of mine (which I could refactor and do a clean PR once your modifications are resolved)

blaggacao commented 6 years ago

Thinking a little further, and that's where the upstream lobbying comes into place: https://github.com/xoes/go-odoo/blob/master/client/client.go When reading the Database Functions, odoo's code should return the uniqe db id at least for proper instance handling as a matter of scriptability... Once we sorted the present PR out, I'll probably propose an upstream PR which I kindly invite you to support then... :wink:

ahuret commented 6 years ago

Thank you for the feedback @blaggacao :-) As you can see on my new master version, I changed the generated go files name with adding the _gen suffix. I also renamed cli folder as go-odoo-cli since the golang binary take the name of the folder. I want to clearly separate cli part with the rest of the api since cli is only a tool to consume api. I removed the binary and added on README a part to explain how to generate it (basically a simple go build ;) ).

Also, I don't understand the way you handle the Config and Session structs since Session is a field of Config in your version.

Also the function below sounds weird to me cause you have to put two times the Config on your function call :

func (c *Config) Login(s Session) error {
    var uid int
    endpointURL := c.HostURL + "/xmlrpc/2/common"
    client, err := xmlrpc.NewClient(endpointURL, c.Transport)
    if err != nil {
        return err
    }
    err = client.Call("authenticate", []interface{}{s.DbName, s.Username, s.Password, ""}, &uid)
    if err != nil {
        return err
    }
    s.UserID = uid
    c.Session = &s
    return err
}

Could you clarify what you have in mind please ? :)

About the CreateDatabase and DropDatabase functions I'll be happy to see your PR when mine will be ok !

blaggacao commented 6 years ago

Basically CreateDatabase & DropDatabase only consume the (my) Config struct, as the db service does not need a user session, but does need the AdminPassword in Config (which you did not consider in your previous code). One could obviously flatten the struct, but i feel it would be semantically not 100% correct.

Have I expressed my thoughts and reasoning understandably?

blaggacao commented 6 years ago

I also renamed cli folder as go-odoo-cli since the golang binary take the name of the folder. I want to clearly separate cli part with the rest of the api since cli is only a tool to consume api. I removed the binary and added on README a part to explain how to generate it (basically a simple go build ;) ).

I think there is one issue with this approach that is: if you do go get github.com/skilld-labs/go-odoo it will not autoamtically install, hoever if you do go get github.com/skilld-labs/go-odoo/cli it will return an error. (or was it go install?)... Haven't verified though...

llonchj commented 6 years ago

@blagacao please have a look at github.com/llonchj/goodo

It is a fork/rework of this project.

I can see @antony360 PR has not been merged for months.

ahuret commented 6 years ago

Hello @llonchj , Thank you for the interest and the work you did! We'll be more than happy to get a PR from you so we can welcome your contribution :) No worry, we're actively maintaining this project, this PR is awaiting on purpose and relatively soon we'll finish this rework and merge it.

blaggacao commented 6 years ago

Hi Guys, I happened to stumble over protobufs (see https://coreos.com/blog/grpc-protobufs-swagger.html for the implicit next-next step). It might be a possibility to dynamically create an odoo client in whatever language it supports (including go) directly from within Odoo (that's the kind of plan). I'm discussion on drafts about this with @yelizariev

In the end, Odoo's built in RPC it's not really kind of future proof (like for the IOT world etc)... Thinking a little outside of the box :stuck_out_tongue_winking_eye:

More to come (and driven by deadlines rather soon :smile:)...

Let me know if you guys would be interested to join in on this direction, and we could discuss how to incorporate your brain treasures into the discussion...

llonchj commented 6 years ago

@antony360 I made a major refactor on the library. There are major breaking changes and still a issue with reports api. Please have a look at github.com/llonchj/goodo

Once it's finished, i will be happy to request a PR into OCA in order to make it a odoo community driver. What are your thoughts?

jbguerraz commented 5 years ago

Hello. @llonchj Nice efforts! Though, I feel the "contribution" way you choosed is a bit weird and kinda wild. You actually didn't fork but copied the codebase, so removing the commit history, and then you started to promote it to others. This looks like stealing a software, not co-creating, contributing. That being said, your refactoring is great @llonchj, we'll be happy to participate to it, migrate our internal stuff to it. Won't you mind open a PR here so we can move ahead in a nice way ? or do you really want to take over and move ahead with your own copy and having us contributing to it by actually forking your copy and so abandoning this repository ?

Providing a GRPC interface to Odoo would be really really great! if there are any news about it @blaggacao we'll be happy to contribute to such initiative.

blaggacao commented 5 years ago

@jbguerraz I'm currently working on https://github.com/xoe-labs/odoo-operator. Once I get into instance (database) handling, I think I'll come soon back here.

llonchj commented 5 years ago

Hi @jbguerraz!

My apologies in advance. It was never my intention to "steal" software. The README.md file stated clearly the origin and also I have been open all the time about my work (#15).

It is ok to me to merge projects.

I am under the impression that has enough breaking changes to the original codebase to break existing builds on projects without versioning or vendoring.

What is your suggestion?

jbguerraz commented 5 years ago

Hello @llonchj :)

It's ok to break those projects without vendoring (I believe there are not much yet ;)). We would be happy to review, merge your MR, and even add you to the team of that repository :1st_place_medal:

ahuret commented 4 years ago

Closing it