Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DB: Fix update-schema to support lxd in non-GOPATH locations #14704

Merged
merged 1 commit into from
Dec 20, 2024
Merged

DB: Fix update-schema to support lxd in non-GOPATH locations #14704

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions lxd/db/schema/update.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@ import (
"fmt"
"os"
"path"
"path/filepath"
"runtime"
"strings"

Expand Down Expand Up @@ -40,9 +39,10 @@ func DotGo(updates map[int]Update, name string) error {
// Passing 1 to runtime.Caller identifies our caller.
_, filename, _, _ := runtime.Caller(1)

// runtime.Caller returns the path after "${GOPATH}/src" when used with `go generate`.
// runtime.Caller returns source file path starting from github.com.
// Translate into relative path to source file.
if strings.HasPrefix(filename, "github.com") {
filename = filepath.Join(os.Getenv("GOPATH"), "src", filename)
filename = strings.TrimPrefix(filename, "github.com/canonical/lxd/lxd/db/")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@markylaing @masnax tells me that its used by micro* too.

Copy link
Member Author

@tomponline tomponline Dec 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@markylaing for context it was broken in this commit 63b017e

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

I dont follow you. It's lxd-generate's job to update these files?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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 :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It happened tail end of yesterday in https://chat.canonical.com/canonical/pl/f1cqkxqdcpnojcu9zi45ie9fkw

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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

}

file, err := os.Create(path.Join(path.Dir(filename), name+".go"))
Expand Down
Loading