DB: Fix update-schema to support lxd in non-GOPATH locations #14704
Conversation
tomponline
changed the title
Signed-off-by: Thomas Parrott <[email protected]>
tomponline
force-pushed
the
tp-lxd-schema
branch
from
42b8351 to
27fb41b
Compare
| if strings.HasPrefix(filename, "github.com") { | ||
| filename = filepath.Join(os.Getenv("GOPATH"), "src", filename) | ||
| filename = strings.TrimPrefix(filename, "github.com/canonical/lxd/lxd/db/") |
There was a problem hiding this comment.
Honestly I'm finding it strange that this is required at all.
As far as I can tell, schema generation is only used by LXD and not any consumers or lxd-generate (microcloud, microceph, etc.), they only use it for query generation.
lxd-generate is directly importing
"github.com/canonical/lxd/lxd/db/cluster"
"github.com/canonical/lxd/lxd/db/node"
to call cluster.SchemaDotGo and node.SchemaDotGo for generating schemas.
I think it'd be easier to reason about if we take this out of lxd-generate and have a small go program call these functions and input the exact filepath we want.
There was a problem hiding this comment.
@markylaing @masnax tells me that its used by micro* too.
There was a problem hiding this comment.
I'm happy to have it reworked, but we need it to be working now (its broken right now and blocking a PR), and I dont have time to focus on a larger rework right now im afraid. But we can follow up with a better refactor if you have time for it?
There was a problem hiding this comment.
@markylaing for context it was broken in this commit 63b017e
There was a problem hiding this comment.
I think it'd be easier to reason about if we take this out of
lxd-generateand have a small go program call these functions and input the exact filepath we want.
I dont follow you. It's lxd-generate's job to update these files?
There was a problem hiding this comment.
We could increase the specificity of if strings.HasPrefix(filename, "github.com") { to check for github.com/canonical/lxd/lxd/db/ if you think that would help with microcloud usage?
There was a problem hiding this comment.
@masnax tells me that its used by micro* too.
micro* may be using query generation but I don't see how it's possible that they're using lxd-generate schema since it doesn't take any arguments, it only generates the LXD schema.
I've just seen the thread for this (I didn't realise it was blocking a PR) so will approve for now and we can discuss more in the new year :)
There was a problem hiding this comment.
It happened tail end of yesterday in https://chat.canonical.com/canonical/pl/f1cqkxqdcpnojcu9zi45ie9fkw
There was a problem hiding this comment.
micro*may be using query generation but I don't see how it's possible that they're usinglxd-generateschema since it doesn't take any arguments, it only generates the LXD schema.
You may well be right, all I got from @masnax so far that gave me that impression was:
One small issue might be with goimports which does not properly handle non-v1/v0 go module packages. But that wouldn't affect LXD, only Micro*, which also use lxd-generate
This is an alternative to #14704 I didn't understand why `runtime.Caller` was required. It was because of where the `SchemaDotGo` functions were defined in the `node` and `cluster` packages. We can instead just split the `go:generate` directives into their respective packages so that invocation there will cause the `schema.go` files to be created in the current directory. As an aside, since `lxd-generate` is used by `micro*` packages, I'd also be in favour of removing the `schema` subcommand from it, as it only supports generating the LXD node or cluster schemas.
No description provided.