add enum support for modelgen

[halfcrazy]

Fix #133

[coveralls]

Pull Request Test Coverage Report for Build 924693681

• 33 of 40 (82.5%) changed or added relevant lines in 1 file are covered.
• 1 unchanged line in 1 file lost coverage.
• Overall coverage increased (+0.1%) to 72.514%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
modelgen/table.go	33	40	82.5%

Files with Coverage Reduction	New Missed Lines	%
modelgen/table.go	1	88.24%

Totals
Change from base Build 922134269:	0.1%
Covered Lines:	2757
Relevant Lines:	3802

---

💛 - Coveralls

[dave-tucker]

Thanks @halfcrazy this looks good! It needs a rebase since #143 has moved this template to a package.

[halfcrazy]

Thanks @halfcrazy this looks good! It needs a rebase since #143 has moved this template to a package.

rebased

[amorenoz]

Thanks @halfcrazy for this PR, it's pretty neat.
Apart from a few comments, I'd like to ask you if you have tested the generated structs against the API. Changing the type might confuse the bindings layer...

[halfcrazy]

Thanks @halfcrazy for this PR, it's pretty neat.
Apart from a few comments, I'd like to ask you if you have tested the generated structs against the API. Changing the type might confuse the bindings layer...

Yes, I have tested play with ovs example.

[amorenoz]

Thanks @halfcrazy for this PR, it's pretty neat.
Apart from a few comments, I'd like to ask you if you have tested the generated structs against the API. Changing the type might confuse the bindings layer...

Yes, I have tested play with ovs example.

I think that is not enough.
There are many places where the reflect.Type of the field is checked. Some of those places will actually need changing in the code.
We're going to need more tests to make sure the core library can cope with those types at least in:

• ovsdb.bindings: ovs2native, native2ovs as well as validateMutation and validateCondition. Also, we need to make sure it works with slices/maps of enums
• mapper (e.g: equality, schema validation, etc) and mapperInfo layer also needs checks and probably code changes.
• from the client (api) perspective we should make sure we can Mutate and Update a typed-enum as well as Create a Condition on them

[dave-tucker]

There are many places where the reflect.Type of the field is checked. Some of those places will actually need changing in the code.

Yeah this is going to be nasty.

Running play_with_ovs results in:

$ go run ./example/play_with_ovs -ovsdb tcp::49154
2021/06/09 18:02:22 Unable to Connect database validation error (1): Mapper Error. Object type main.Bridge contains field FailMode ([]main.BridgeFailMode) ovs tag fail_mode: Wrong type, column expects []string
exit status 1

We're going to need to use reflect.Kind to compare types, where we're currently checking the equality of reflect.Type

[dave-tucker]

Because diving in deep on reflection isn't the easiest thing for new contributors, I've opened #151 which, when applied, makes the play_with_ovs example work (I adjusted it to set the bridges protocol fields too). I'm still not 100% confident with the implementation and testing in place so I'm leaving it as a draft for now.

[halfcrazy]

Because diving in deep on reflection isn't the easiest thing for new contributors, I've opened #151 which, when applied, makes the play_with_ovs example work (I adjusted it to set the bridges protocol fields too). I'm still not 100% confident with the implementation and testing in place so I'm leaving it as a draft for now.

You are awesome

[halfcrazy]

change type A string to type A = string seem to solve the problem.

https://github.com/golang/proposal/blob/master/design/18130-type-alias.md

$ go run ./example/play_with_ovs -ovsdb unix:`pwd`/db.sock
Silly game of stopping this app when a Bridge with name "stop" is monitored !

 Enter a Bridge Name : bridge
Bridge Addition Successful :  524f8b36-6d1e-42d5-8ca3-cb6b600cf4c8

 Enter a Bridge Name : Current list of bridges:
UUID: 524f8b36-6d1e-42d5-8ca3-cb6b600cf4c8  Name: bridge

[dave-tucker]

@halfcrazy good find! I read that yesterday but the difference between type T1 string and type T1 = string didn't log in my brain 😆

As you pointed out, as the type alias is invisible at runtime we can use this without adding any more reflection 🎉 . There is however a trade-off:

https://play.golang.org/p/FUWw6m9Wbiq

In that example, I can assign my type C = string to a field that is supposed to be type A = string. At least with the type alias we give the user a "hint" as to the values they are supposed to use, but the compiler isn't going to help you out.

However, having Enums implemented outweighs this, because we can always change the way enums are generated in future to be more strongly typed.

So, this LGTM.

@halfcrazy could you please squash your commits?
I'll let @amorenoz review this one (again) before we merge

[halfcrazy]

@halfcrazy good find! I read that yesterday but the difference between type T1 string and type T1 = string didn't log in my brain 😆

As you pointed out, as the type alias is invisible at runtime we can use this without adding any more reflection 🎉 . There is however a trade-off:

https://play.golang.org/p/FUWw6m9Wbiq

In that example, I can assign my type C = string to a field that is supposed to be type A = string. At least with the type alias we give the user a "hint" as to the values they are supposed to use, but the compiler isn't going to help you out.

However, having Enums implemented outweighs this, because we can always change the way enums are generated in future to be more strongly typed.

So, this LGTM.

@halfcrazy could you please squash your commits?
I'll let @amorenoz review this one (again) before we merge

Done. This is a trade-off we lose the compile-time check, but we don't need to touch reflect code, it's acceptable. We may refactor later.

[amorenoz]

The tradeoff seems reasonable to me. Thanks @halfcrazy for the work.
Created #154 to work on the strong enum support.
LGTM