Graph Loader initial implementation #224

kevinrizza · 2020-03-24T19:13:37Z

*Create new graph type that can describe channel graphs of packages
*Create initial implementation of loading it from existing sqlite db

This graph implementation should be used as a declarative structure to define the update graph for both input and output in the future

Bowenislandsong

I think this is very nice. Although, I am only using graph.go.

exdx · 2020-03-24T19:25:47Z

pkg/sqlite/graphloader.go

+	PackageName string
+}
+
+// TODO


nit: remove

exdx · 2020-03-24T19:26:11Z

pkg/sqlite/graphloader.go

+	}, nil
+}
+
+// TODO get bundle path and version


nit: remove

exdx · 2020-03-24T19:26:36Z

pkg/sqlite/graphloader_test.go

+	"github.com/operator-framework/operator-registry/pkg/registry"
+)
+
+func createLoadedTestDb(t *testing.T) (*sql.DB, func()) {


woo nice test 🎉

ecordell · 2020-03-24T19:20:08Z

pkg/registry/graph.go

+	ReplacesBundles []OperatorBundle
+	Replaces        []BundleRef


Suggested change

ReplacesBundles []OperatorBundle

Replaces []BundleRef

Replaces []*OperatorBundle

Why not this?

After reviewing the rest, I think this could also be:

Suggested change

ReplacesBundles []OperatorBundle

Replaces []BundleRef

Replaces map[string]*OperatorBundle

or:

Suggested change

ReplacesBundles []OperatorBundle

Replaces []BundleRef

Replaces map[BundleRef]*OperatorBundle

ecordell · 2020-03-24T19:20:40Z

pkg/registry/graph.go

+
+type BundleRef struct {
+	BundlePath string
+	Version    string //semver string


Why not type this as a semver field?

ecordell · 2020-03-24T19:21:08Z

pkg/registry/graph.go

+
+type OperatorBundle struct {
+	BundlePath      string
+	Version         string // semver string


Why not typed?

ecordell · 2020-03-24T19:22:47Z

pkg/sqlite/graphloader.go

+	return channels, nil
+}
+
+func getBundle(bundles []registry.OperatorBundle, name string) *registry.OperatorBundle {


it really seems like this could be a map?

ecordell · 2020-03-24T19:23:50Z

pkg/sqlite/graphloader.go

+	return nil
+}
+
+func getHeadBundleRefForChannel(bundles []registry.OperatorBundle) *registry.BundleRef {


This assumes an already valid graph, which might not be true of the places you're intending to call this

This specific implementation is reading from the sql layer. The assumption here is that the graph is valid because we wouldn't persist it in the first place otherwise.

That being said, I can totally as we flesh this out attempting to do some generation validation. Maybe that's what we should tackle next, and when the insert is written we include that as part of this function?

ecordell · 2020-03-24T19:24:31Z

pkg/sqlite/query.go

@@ -113,6 +113,76 @@ func (s *SQLQuerier) GetPackage(ctx context.Context, name string) (*registry.Pac
 	return pkg, nil
 }

+func (s *SQLQuerier) GetDefaultPackage(ctx context.Context, name string) (string, error) {


nit: should this be GetDefaultChannel?

Also - is the query interface updated with these?

latest updated includes the interface update

ecordell · 2020-03-24T19:27:18Z

pkg/registry/graph.go

+type OperatorBundle struct {
+	BundlePath      string
+	Version         string // semver string
+	CsvName         string


In some newer parts of this codebase, we have moved away from CsvName to BundleName or Identifier

I think the intention of this type is to explicitly be an aggregate identifier. At some point CsvName could be deprecated entirely. For now they are equivalent though, and I think it's more likely that this field would be removed than refer to a different concept. Wdyt?

Bowenislandsong · 2020-03-24T19:38:03Z

pkg/registry/graph.go

+type OperatorBundle struct {
+	BundlePath      string
+	Version         string // semver string
+	CsvName         string


WDYT if we replace the first 3 fields with bundleRef? This way we define how we refer to a bundle in one place?

*Create new graph type that can describe channel graphs of packages *Create initial implementation of loading it from existing sqlite db

exdx · 2020-03-25T13:30:23Z

pkg/registry/graph.go

+	CsvName    string
+}
+
+func (b *BundleKey) IsEmpty() bool {


Since we're using the BundleKey struct as a key in the map, should we add another method to check if two bundle keys are identical? From https://blog.golang.org/maps (where KeyType may be any type that is comparable) .

I guess we don't have to for the purposes of defining the map (Go does this for us) but it may be a helpful utility function to have alongside b.IsEmpty(). b.IsEqual(a).

I think that's a fair thing to suggest, but perhaps it should just be included when something that needs to do the comparison is written?

exdx · 2020-03-25T13:33:38Z

pkg/registry/types.go

@@ -76,6 +76,18 @@ type ChannelEntry struct {
 	Replaces    string
 }

+// ChannelEntryNode is a denormalized node in a channel graph annotated with additional entry level info
+type ChannelEntryNode struct {


I think the naming between ChannelEntry and ChannelEntryNode is a little confusing. Based on the comments they are similar, one just had more details in it. Maybe we can call it ChannelEntryAnnotated?

exdx · 2020-03-25T13:34:43Z

pkg/sqlite/graphloader.go

+// GraphLoader generates a graph
+// GraphLoader supports multiple different loading schemes
+// GraphLoader from SQL, GraphLoader from old format (filesystem), GraphLoader from SQL + input bundles
+type GraphLoader interface {


Should we move this interface out of this package as part of this PR?

Bowenislandsong · 2020-03-25T13:36:36Z

pkg/registry/graph.go

+
+type Channel struct {
+	Head     BundleKey
+	Replaces map[BundleKey]map[BundleKey]struct{}


What happened here? Can we define map[BundleKey]map[BundleKey]struct{}? like ReplaceMap?

What about nodes ?

exdx · 2020-03-25T13:38:21Z

pkg/sqlite/graphloader.go

+}
+
+// GraphFromEntries builds the graph from a set of channel entries
+func (g *SQLGraphLoader) GraphFromEntries(channelEntries []registry.ChannelEntryNode) (map[string]registry.Channel, error) {


I'm wondering if we should haveGraphFromEntries be on the GraphLoader interface as well - we want all the different loaders to be able to provide this functionality as well right

Or maybe we can have a separate Grapher interface 😄

I actually think we explicitly don't want other graphloaders to have this method. This is really specific to the sqlite layer, other loaders wouldn't be aware of this aggregation at all.

todo: squash commits

pkg/sqlite/graphloader.go

exdx · 2020-03-25T16:16:58Z

pkg/sqlite/graphloader.go

+}
+
+// GraphFromEntries builds the graph from a set of channel entries
+func (g *SQLGraphLoader) GraphFromEntries(channelEntries []registry.ChannelEntryAnnotated) (map[string]registry.Channel, error) {


suggestion: can we make this a function instead of a method of the SQLGraphLoader? the function doesn't use any of the SQLGraphLoader data. This makes the func easier to use in other pkgs

seems reasonable

exdx · 2020-03-25T16:17:49Z

/lgtm

exdx · 2020-03-25T16:31:17Z

/lgtm

openshift-bot · 2020-03-25T16:44:59Z

/retest

Please review the full test history for this PR and help us cut down flakes.

benluddy · 2020-03-25T16:46:53Z

/lgtm

openshift-ci-robot · 2020-03-25T16:46:57Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: benluddy, exdx, kevinrizza

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [kevinrizza]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-bot · 2020-03-25T16:57:59Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-03-25T17:11:18Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-03-25T17:24:19Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-03-25T17:37:06Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-ci-robot · 2020-03-25T18:04:28Z

@kevinrizza: The following test failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
ci/prow/images	`7ca004d`	link	`/test images`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

openshift-ci-robot requested review from Bowenislandsong and exdx March 24, 2020 19:13

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 24, 2020

Bowenislandsong reviewed Mar 24, 2020

View reviewed changes

exdx reviewed Mar 24, 2020

View reviewed changes

pkg/sqlite/graphloader.go Outdated

PackageName string

}

// TODO

Copy link

Member

exdx Mar 24, 2020

Choose a reason for hiding this comment

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

nit: remove

kevinrizza reacted with thumbs up emoji

exdx reviewed Mar 24, 2020

View reviewed changes

pkg/sqlite/graphloader.go Outdated

}, nil

}

// TODO get bundle path and version

Copy link

Member

exdx Mar 24, 2020

Choose a reason for hiding this comment

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

nit: remove

exdx reviewed Mar 24, 2020

View reviewed changes

ecordell reviewed Mar 24, 2020

View reviewed changes

Bowenislandsong reviewed Mar 24, 2020

View reviewed changes

Graph Loader initial implementation

ff4daf8

*Create new graph type that can describe channel graphs of packages *Create initial implementation of loading it from existing sqlite db

kevinrizza force-pushed the graph-loader branch from d5d9944 to ff4daf8 Compare March 25, 2020 12:13

Update graph format for runtime concerns

70d01de

exdx reviewed Mar 25, 2020

View reviewed changes

Bowenislandsong reviewed Mar 25, 2020

View reviewed changes

exdx reviewed Mar 25, 2020

View reviewed changes

address pr feedback

67704c5

todo: squash commits

benluddy reviewed Mar 25, 2020

View reviewed changes

pkg/sqlite/graphloader.go Outdated Show resolved Hide resolved

use query interface in sql graph loader

235c245

exdx approved these changes Mar 25, 2020

View reviewed changes

openshift-ci-robot assigned exdx Mar 25, 2020

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 25, 2020

Update graphFromEntries to be a private function

7ca004d

openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Mar 25, 2020

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 25, 2020

openshift-ci-robot assigned benluddy Mar 25, 2020

openshift-merge-robot merged commit d2e8855 into operator-framework:master Mar 25, 2020

	ReplacesBundles []OperatorBundle
	Replaces []BundleRef
	Replaces []*OperatorBundle

	ReplacesBundles []OperatorBundle
	Replaces []BundleRef
	Replaces map[string]*OperatorBundle

	ReplacesBundles []OperatorBundle
	Replaces []BundleRef
	Replaces map[BundleRef]*OperatorBundle

Graph Loader initial implementation #224

Graph Loader initial implementation #224

Conversation

kevinrizza commented Mar 24, 2020

Bowenislandsong left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

exdx Mar 25, 2020 • edited

Choose a reason for hiding this comment

Bowenislandsong Mar 25, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

exdx commented Mar 25, 2020

exdx commented Mar 25, 2020

openshift-bot commented Mar 25, 2020

benluddy commented Mar 25, 2020

openshift-ci-robot commented Mar 25, 2020

openshift-bot commented Mar 25, 2020

openshift-bot commented Mar 25, 2020

openshift-bot commented Mar 25, 2020

openshift-bot commented Mar 25, 2020

openshift-ci-robot commented Mar 25, 2020

exdx Mar 25, 2020 •

edited

Bowenislandsong Mar 25, 2020 •

edited