Reconcile OperatorSource type #4

tkashem · 2018-09-24T21:52:02Z

Reconcile OperatorSource type by doing the following in order:

Pull down operator manifests from the specified app registry server and store them locally.
Create a corresponding CatalogSourceConfig instance.

Gopkg.lock

Gopkg.toml

Gopkg.lock

cmd/marketplace-operator/main.go

aravindhp · 2018-09-25T13:59:05Z

pkg/apis/marketplace/v1alpha1/types.go

+	// Type of operator source
+	Type string `json:"type"`
+
+	// Source points to the URL from where operator manifests can be fetched


Endpoint points to...

aravindhp · 2018-09-25T17:09:31Z

pkg/operatorsource/reconciler.go

+
+// Given a name of an instance of OperatorSource type, this function returns the name of the associated CatalogSourceConfig type object
+func getCatalogSourceConfigName(operatorSourceName string) string {
+	return fmt.Sprintf("os-%s", operatorSourceName)


Please change the os prefix to something else given that is such an overloaded (Operating System, OpenShift etc) abbreviation. Plus I think it is an internal Go package name. I suggest opsrc or some such thing.

Nit: you could go a step further and make the prefix a constant like we have for CatalogSourceConfig.

aravindhp · 2018-09-25T17:13:41Z

pkg/operatorsource/reconciler.go

+	return fmt.Sprintf("os-%s", operatorSourceName)
+}
+
+func (r *reconciler) IsAlreadyReconciled(os *v1alpha1.OperatorSource) (bool, error) {


Same comment as above for the os variable name. I had to do a double take to realize you mean OperatorSource :-)

i like opsrc :)

pkg/operatorsource/reconciler.go

aravindhp · 2018-09-25T17:22:26Z

pkg/operatorsource/reconciler_test.go

+)
+
+func TestReconcile(t *testing.T) {
+	os.Setenv("KUBERNETES_CONFIG", getKubeConfigFilePath())


Same request as above

taken care of

What if the config doesn't exist? Won't it be better to catch that and make the test fail gracefully ?

pkg/stub/handler.go

tkashem · 2018-09-25T20:25:29Z

@aravindhp let me know if you have more comments

aravindhp

Everything looks good except for the two unresolved comments.

aravindhp · 2018-09-25T21:41:43Z

pkg/operatorsource/reconciler_test.go

+)
+
+func TestReconcile(t *testing.T) {
+	os.Setenv("KUBERNETES_CONFIG", getKubeConfigFilePath())


What if the config doesn't exist? Won't it be better to catch that and make the test fail gracefully ?

aravindhp · 2018-09-26T14:44:47Z

LGTM to me except for #4 (comment)

specified app registry server and making it available to the cluster

ravisantoshgudimetla · 2018-09-27T02:26:04Z

cmd/marketplace-operator/main.go

@@ -31,10 +31,12 @@ func main() {
 	if err != nil {
 		logrus.Fatalf("failed to get watch namespace: %v", err)
 	}
-	resyncPeriod := time.Duration(5) * time.Second
-	logrus.Infof("Watching %s, %s, %s", resource, catalogSourceConfigKind, namespace)
+


Remove blank lines.

resyncPeriod is shared by the two types here, hence it gets its own spacing.

ravisantoshgudimetla · 2018-09-27T02:32:27Z

pkg/appregistry/appregistry.go

@@ -0,0 +1,48 @@
+package appregistry


This whole file could be named as factory.go

Yeah, good point. I thought about it. I also like the idea of a {package name}.go file which has the entry level function for the given package. NewFactory is the entry level function one must call to use the appregistry package.

ravisantoshgudimetla · 2018-09-27T02:47:05Z

pkg/appregistry/adapter.go

+	mediaType string = "helm"
+)
+
+// This interface (internal to this package) encapsulates nitty gritty details of go-appr client bindings


Comment could have been a bit more clear on why 3 methods.

That's all we need for this iteration. Do you want more clarity on why this interface exists? I don't think we need to explain why an interface has N number of methods.

ravisantoshgudimetla · 2018-09-27T02:50:45Z

pkg/appregistry/adapter.go

+}
+
+func (a *apprApiAdapterImpl) ListPackages() (appr_models.Packages, error) {
+	params := appr_package.NewListPackagesParams()


Don't we allow listing packages within a namespace? I think we should allow some filtering here

I already have a new PR in review that allows to filter by namespace.

ravisantoshgudimetla · 2018-09-27T03:01:13Z

pkg/appregistry/appregistry_test.go

+func TestRetrieveAll(t *testing.T) {
+	factory := appregistry.NewClientFactory()
+
+	client, err := factory.New("appregistry", "http://localhost:5000/cnr")


Not sure, if the assumption is we have some service running on 5000? Or did you mock it somewhere?

This test is hanging around here for debugging purposes. It will be replaced by a unit test.

ravisantoshgudimetla · 2018-09-27T03:02:25Z

pkg/appregistry/appregistry_test.go

@@ -0,0 +1,22 @@
+package appregistry_test


Why a new package for testing?

I am treating this test as a black box test, hence a new package for test(s) and explicit import of package being tested.

ravisantoshgudimetla · 2018-09-27T03:10:33Z

pkg/appregistry/client.go

+		return nil, err
+	}
+
+	list := make([]*OperatorMetadata, len(packages))


operatorMetaDataList probably would have made more sense.

I have to implement a operatorMetaDataList. This is not a k8s type.

ravisantoshgudimetla · 2018-09-27T03:12:04Z

pkg/appregistry/client.go

+	for i, pkg := range packages {
+		manifest, err := c.RetrieveOne(pkg.Name, pkg.Default)
+		if err != nil {
+			return nil, err


Do you want to return error, if retrieving one package is erroring? It could that one package is misbehaving, probably aggregated errorlist would make more sense.

This is a rudimentary version geared toward the proof of concept. I am working on prod ready reconciliation process now.

ravisantoshgudimetla · 2018-09-27T03:19:58Z

pkg/appregistry/decoder.go

+	"io"
+)
+
+type blobDecoder interface {


I think you could have avoided interface altogether here and just used the struct.

On the contrary, I can easily mock this interface when I am testing the unit that depends on this interface, Look at TestRetrieveOne_PackageExists_SuccessExpected. Not shy to use interfaces :)

ravisantoshgudimetla · 2018-09-27T03:24:21Z

pkg/appregistry/mock_adapter.go

+// Source: ./pkg/appregistry/adapter.go
+
+// Package appregistry is a generated GoMock package.
+package appregistry


Why is this here? Either we shouldn't checkin the generated code or if we absolutely need it we should create a script to generate it. I thought appregistry is not generated

ravisantoshgudimetla · 2018-09-27T03:27:44Z

pkg/operatorsource/datastore.go

+	}
+}
+
+type hashmapDatastore struct {


Please change the name to something appropriate like DataStore

Hmm, do you have any suggestions? MemoryDatastore, TransientDatastore or DummyDatastore maybe? :)

ravisantoshgudimetla · 2018-09-27T03:28:10Z

pkg/operatorsource/datastore.go

+}
+
+type hashmapDatastore struct {
+	list map[string]*appregistry.OperatorMetadata


Change list to something appropriate.

hmm, I can rename it to packages. That is not so generic as list

ravisantoshgudimetla · 2018-09-27T03:35:46Z

pkg/stub/handler.go

 }

 type Handler struct {
-	// Fill me
+	operatorSourceHanlder operatorsource.Handler


good catch :)

openshift-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Sep 24, 2018

tkashem force-pushed the registry branch from 8a4ab93 to 186723e Compare September 24, 2018 21:53

tkashem requested review from aravindhp and ravisantoshgudimetla September 24, 2018 21:54

tkashem force-pushed the registry branch from 6669fa8 to 374e69f Compare September 24, 2018 22:03

aravindhp requested changes Sep 25, 2018

View reviewed changes

tkashem force-pushed the registry branch 4 times, most recently from b40a36a to 53d849d Compare September 25, 2018 20:23

aravindhp requested changes Sep 25, 2018

View reviewed changes

tkashem force-pushed the registry branch from 53d849d to 4932f4d Compare September 26, 2018 04:13

Reconcile operatorsource type by pullng down operator manifest from

926e831

specified app registry server and making it available to the cluster

tkashem force-pushed the registry branch from 4932f4d to 926e831 Compare September 26, 2018 15:37

aravindhp approved these changes Sep 26, 2018

View reviewed changes

aravindhp merged commit e304bbc into operator-framework:master Sep 26, 2018

ravisantoshgudimetla reviewed Sep 27, 2018

View reviewed changes

Reconcile OperatorSource type #4

Reconcile OperatorSource type #4

Conversation

tkashem commented Sep 24, 2018

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

tkashem Sep 25, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tkashem commented Sep 25, 2018

aravindhp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aravindhp commented Sep 26, 2018

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

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

tkashem Sep 25, 2018 •

edited

Loading