Skip to content

(entitySource) Use catalogd provided content for sourcing entities #182

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.

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

Merged
merged 2 commits into from
May 8, 2023

Conversation

anik120
Copy link
Contributor

@anik120 anik120 commented Apr 24, 2023

This PR replaces OLM v0 catalog operator with https://github.com/operator-framework/catalogd as the source of entities for deppy.

Addresses #161

@anik120 anik120 force-pushed the catalogd-integration branch from 29241c3 to 0469e7f Compare April 24, 2023 22:26
@anik120 anik120 force-pushed the catalogd-integration branch from 0469e7f to cf3f52e Compare April 24, 2023 22:46
@anik120 anik120 mentioned this pull request Apr 24, 2023
7 tasks
Copy link

@stevekuznetsov stevekuznetsov left a comment

Choose a reason for hiding this comment

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

Looks like an OLMv0 end-to-end test was removed - is there a new one for this integration?

Version string `json:"version"`
}
tmp := pkg{}
if err := json.Unmarshal(prop.Value, &tmp); err != nil {

Choose a reason for hiding this comment

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

What's the meaning of this marshal/unmarshal step? Do we not trust which fields are in the prop.Value? Can better validation be added to bundle.Spec.Properties? Why would it ever be valid for someone to create a bundle with an invalid property?

Copy link
Contributor

Choose a reason for hiding this comment

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

Mistakes happen? And we should always validate user-derived input?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So right now we just throw in whatever we're getting from the declCfg into the CR, which ends up looking something like this:

spec:
  catalogSource: catalogsource-sample
  image: quay.io/operatorhubio/argocd-operator@sha256:5f4533de114e9301e836155190dc2c66a969f55085af03a4959a2fb0af74e1f4
  package: argocd-operator
  properties:
  - type: olm.gvk
    value: ewogICAgICAgICAgICAgICAgImdyb3VwIjogImFyZ29wcm9qLmlvIiwKICAgICAgICAgICAgICAgICJraW5kIjogIkFwcFByb2plY3QiLAogICAgICAgICAgICAgICAgInZlcnNpb24iOiAidjFhbHBoYTEiCiAgICAgICAgICAgIH0=
  - type: olm.gvk
    value: ewogICAgICAgICAgICAgICAgImdyb3VwIjogImFyZ29wcm9qLmlvIiwKICAgICAgICAgICAgICAgICJraW5kIjogIkFwcGxpY2F0aW9uIiwKICAgICAgICAgICAgICAgICJ2ZXJzaW9uIjogInYxYWxwaGExIgogICAgICAgICAgICB9
  - type: olm.gvk
    value: ewogICAgICAgICAgICAgICAgImdyb3VwIjogImFyZ29wcm9qLmlvIiwKICAgICAgICAgICAgICAgICJraW5kIjogIkFwcGxpY2F0aW9uU2V0IiwKICAgICAgICAgICAgICAgICJ2ZXJzaW9uIjogInYxYWxwaGExIgogICAgICAgICAgICB9
  - type: olm.gvk
    value: ewogICAgICAgICAgICAgICAgImdyb3VwIjogImFyZ29wcm9qLmlvIiwKICAgICAgICAgICAgICAgICJraW5kIjogIkFyZ29DRCIsCiAgICAgICAgICAgICAgICAidmVyc2lvbiI6ICJ2MWFscGhhMSIKICAgICAgICAgICAgfQ==
  - type: olm.gvk
    value: ewogICAgICAgICAgICAgICAgImdyb3VwIjogImFyZ29wcm9qLmlvIiwKICAgICAgICAgICAgICAgICJraW5kIjogIkFyZ29DREV4cG9ydCIsCiAgICAgICAgICAgICAgICAidmVyc2lvbiI6ICJ2MWFscGhhMSIKICAgICAgICAgICAgfQ==
  - type: olm.package
    value: ewogICAgICAgICAgICAgICAgInBhY2thZ2VOYW1lIjogImFyZ29jZC1vcGVyYXRvciIsCiAgICAgICAgICAgICAgICAidmVyc2lvbiI6ICIwLjUuMCIKICAgICAgICAgICAgfQ==
  relatedImages:
  - image: gcr.io/kubebuilder/kube-rbac-proxy:v0.8.0
    name: ""
  - image: quay.io/argoprojlabs/argocd-operator@sha256:2f5c0d4567607266ccacb91d8c2e3b18c2afe0edaf55855dcb1a06b02173b520
    name: ""
  - image: quay.io/operatorhubio/argocd-operator@sha256:5f4533de114e9301e836155190dc2c66a969f55085af03a4959a2fb0af74e1f4
    name: ""

We'd discussed correcting this and changing the plumbing to make sure we throw in unmarshalled structs there instead, but again, that was superseded with operator-framework/catalogd#38. Hence the tap dance with marshal/unmarshal: just a stop gap so that we're not sweating over details that are going to change soon.

Choose a reason for hiding this comment

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

@tmshort if it's required, the validation should occur at creation or mutation of the object, not downstream every time the value is used.

Choose a reason for hiding this comment

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

@anik120 is there an understanding of how long that effort will take? Leaving the software in a more stable spot while the change happens might be useful if it's a long-running effort?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's definitely a "the very next step" for the project. We'd planned it for milestone 3 but it got pushed out in favor of us focusing our effort on the integration for m3, so I'd say milestone 4 is a pretty accurate timeline for when it's about to happen. (We can probably do a v0.2.0 release after m4 and in m5 we can update this integration with the v0.2.0 release, but don't quote me on that yet I'd like to make sure others working in the project are in agreement with "we can do these things we're promising in that timeline" too :) ). But the gist of it is, these are going to change pretty soon.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Mistakes happen? And we should always validate user-derived input?

@tmshort if it's required, the validation should occur at creation or mutation of the object, not downstream every time the value is used.

All makes sense. I've made a note of it here so that we don't miss this

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure I'm understanding the need to unmarshal/marshal. I know that deppy wants maps of properties, not lists, so I would expect some handling to do that conversion. But isn't this just a matter of collecting up the properties that have the same type into map[string][]json.Message, and then building the deppy entities from that?

return nil
}

func jsonMarshal(p interface{}) ([]byte, error) {

Choose a reason for hiding this comment

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

I've never quite seen a JSON marshal method this intricate - why is it necessary over json.Marshal? What's special about encoding HTML?

Copy link
Member

Choose a reason for hiding this comment

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

json.Marshal always escapes for HTML.

Copy link
Member

Choose a reason for hiding this comment

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

Sorry! Read right over the fact that you asked specifically about the HTML aspect. HTML encoding subs out <, >, and & for unicode escape sequences, which isn't super human-friendly. In the case of our FBC code in operator-registry (where I suspect @anik120 found this?), this is important because we expect humans to be looking at the output.

In this case, I'm not sure whether humans will ever see anything downstream of this marshaling. Theoretically, I suppose it could show up in resolution error messages?

Choose a reason for hiding this comment

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

Wouldn't we expect to HTML-escape just before putting the output into a webpage or something? I am still lost as to why we're not operating over raw values here.

Copy link
Member

Choose a reason for hiding this comment

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

This is disabling HTML escaping, which is the hardcoded only option in json.Marshal.

However, I think we could probably avoid doing any json marshaling in this entity source. I mentioned in a different comment that there is some impedance mismatch between FBC properties ([]Property) and deppy properties (map[string]string, I think?), which does seem awkward, but since we already have json.RawMessage, it seems like we could do the conversion without needing to marshal/unmarshal anything.

Choose a reason for hiding this comment

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

Huh, TIL. For whom are we disabling this? What consumer needs JSON to not be HTML-escaped?

Copy link
Member

Choose a reason for hiding this comment

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

We do something like this to disable HTML encoding in opm render because we expect humans to be looking at the output.

Here, the resolver will see this. The only place I could see a human being exposed to resolver input is in resolver error messages, and it's conceivable that property values will end up there.

Choose a reason for hiding this comment

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

I think I'm still really lost. Even in the resolver if I'm reading this code correctly a json.Unmarshal happens first so it would be back to raw values ... In user-facing output of JSON (which in and of itself is a bit of an oxymoron) this makes sense but I think unless there's some clear need, starting with a simple call to json.Marshal makes sense ...

Copy link
Member

Choose a reason for hiding this comment

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

👍 Seems reasonable to me to start with json.Marshal and re-evaluate if there's a need.

Copy link
Contributor

@everettraven everettraven left a comment

Choose a reason for hiding this comment

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

First round - most of my comments are around trying to understand the decision to go with a static entity source implementation rather than a dynamic one.

@@ -63,6 +44,8 @@ var _ = Describe("Operator Install", func() {
err := c.Create(ctx, operator)
Expect(err).ToNot(HaveOccurred())

// TODO dfranz: This test currently relies on the hard-coded CatalogSources found in bundle_cache.go
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this comment be removed instead of added back since the hard-coded catalog sources are gone?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Consolidating comments about e2e here.

@stevekuznetsov : "Looks like an OLMv0 end-to-end test was removed - is there a new one for this integration?"

I realized after removing the hardcoded entity source that the v0 CatalogSource integration was using the v0 OLM installation to create a CatalogSource with an index image, but then for us to include prod grade e2e tests we probably have to set up the e2e test infrastructure first. That means

  • Discuss if we want to install catalogd v0.0.1 as part of the operator-controller installation
  • Set up test infrastructure like a mock image registry, that can be used to build and post an index image into so that the tests can pull from it etc.

My first instinct is to add the hardcoded entitySource back, so that the deppy tests can go on with the hardcoded entitySource, and we don't increase the scope of this PR with "set up e2e test infrastructure for this project", but then if we feel like we must do it as a part of this PR then I'm happy to do it, with an agreement that it'll increase the scope of this PR.

cc: @joelanford

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Discuss if we want to install catalogd v0.0.1 as part of the operator-controller installation

My guess for this is "yes we want this to be included in this PR, just like we install rukpak", but I'll wait for others to chime in first, to make sure we're comfortable including v0.1.0 release of catalogd as a part of the operator-contorller installation

Copy link
Member

Choose a reason for hiding this comment

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

Discuss if we want to install catalogd v0.0.1 as part of the operator-controller installation

Yes, 100%.

Set up test infrastructure like a mock image registry, that can be used to build and post an index image into so that the tests can pull from it etc.

Agree with doing this separately. There's a whole bunch wrapped up in this question:

  1. Should we do the catalog.spec.source union type first so that we don't need all the image infrastructure everywhere we want to test it?
  2. What tests should we put in catalogd vs operator-controller?
  3. What's the appropriate layer for the tests? Unit or e2e? Or both?

Copy link
Member

@joelanford joelanford left a comment

Choose a reason for hiding this comment

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

This isn't really what I had in mind when we talked about it before. I was anticipating the dynamic entity source that Bryce was alluding to in comments. Having said that, I'm not necessarily opposed to the "read everything into memory at init time" approach because it technically seems like it will meet the bar set out in the demo script.

I do think we should setup the entity source once and remove the if r.Resolver == nil block in the reconciler though. I know doing that may necessitate making this more dynamic though.

.gitignore Outdated
@@ -1,3 +1,4 @@
vendor
Copy link
Member

Choose a reason for hiding this comment

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

Is there some reason that this needs to be gitignore'd? I don't think anything in the repo generates a vendor dir, so I don't think we ever expect it to exist.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You have to go mod vendor to build the manager binary, and you either have to delete it every time you're about to push to your PR or never do git add . :) I don't think it hurts to keep it here

Copy link
Member

Choose a reason for hiding this comment

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

You have to go mod vendor to build the manager binary

I don't have to do that...

Version string `json:"version"`
}
tmp := pkg{}
if err := json.Unmarshal(prop.Value, &tmp); err != nil {
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure I'm understanding the need to unmarshal/marshal. I know that deppy wants maps of properties, not lists, so I would expect some handling to do that conversion. But isn't this just a matter of collecting up the properties that have the same type into map[string][]json.Message, and then building the deppy entities from that?

return nil
}

func jsonMarshal(p interface{}) ([]byte, error) {
Copy link
Member

Choose a reason for hiding this comment

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

json.Marshal always escapes for HTML.

@anik120 anik120 force-pushed the catalogd-integration branch from cf3f52e to 4d13185 Compare April 26, 2023 22:23
@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 26, 2023
Comment on lines +36 to +43
resultSet := input.EntityList{}
entities, err := getEntities(ctx, es.client)
if err != nil {
return nil, err
}
for _, entity := range entities {
if filter(&entity) {
resultSet = append(resultSet, entity)
}
}
return resultSet, nil
Copy link
Member

Choose a reason for hiding this comment

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

Nit: in Filter and GroupBy, is it possible to call Iterate with an iterate lambda function that implements the filter and groupBy logic? That way we don't have to repeat ourselves quite as much (and thus reduce the chance for drift between implementations)

Comment on lines 104 to 98
type pkg struct {
Name string `json:"packageName"`
Version string `json:"version"`
}
tmp := pkg{}
if err := json.Unmarshal(prop.Value, &tmp); err != nil {
return nil, err
}
pkgValue, err := jsonMarshal(tmp)
if err != nil {
return nil, err
}
props[property.TypePackage] = string(pkgValue)
Copy link
Member

Choose a reason for hiding this comment

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

This needs a comment explaining what's going on. To my eye, it's literally unmarshalling and then re-marshaling, which seems like a no-op. Why not consolidate this down to:

case property.TypePackage:
	props[property.TypePackage] = string(prop.Value)

Better yet, I'd imagine something more like this:

valsByType := map[string][]json.RawMessage{}
for _, prop := range bundle.Spec.Properties {
	valsByType[prop.Type] = append(valsByType[prop.Type], prop.Value)
}
entityProps := map[string]string{}
for typ, vals := range valsByType {
	switch typ {
	case property.TypePackage:
		// We expect exactly on "olm.package" property
		if len(vals) != 1 { return err }
		entityProps[typ] = string(vals[0])
	default:
		// All other properties can have 0 or more values, so always use a list
		entityProps[typ] = fmt.Sprintf("[%s]", bytes.Join(vals, []byte(",")))
	}
}

Copy link
Member

@joelanford joelanford left a comment

Choose a reason for hiding this comment

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

Nice update Anik! This is shaping up pretty well, I think.

return nil
}

func jsonMarshal(p interface{}) ([]byte, error) {

Choose a reason for hiding this comment

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

Huh, TIL. For whom are we disabling this? What consumer needs JSON to not be HTML-escaped?

}
return nil
}

func loadFromEntity[T interface{}](entity *input.Entity, propertyName string, required propertyRequirement) (T, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Not related to this PR, but it would be awesome if you could change this to [T any] =D

@anik120 anik120 force-pushed the catalogd-integration branch from 4d13185 to d3a1105 Compare April 27, 2023 19:16
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 27, 2023
@anik120 anik120 force-pushed the catalogd-integration branch 3 times, most recently from 61d29dc to 1d5268e Compare April 27, 2023 19:31
@anik120 anik120 force-pushed the catalogd-integration branch 2 times, most recently from 088c0ae to 07e1b63 Compare April 27, 2023 20:16
Name: "test-catalog",
},
Spec: catalogd.CatalogSourceSpec{
Image: "quay.io/operatorhubio/catalog:latest",
Copy link
Member

Choose a reason for hiding this comment

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

This is a very large catalog to use in an e2e. Okay for now, but can you add a TODO comment and an issue in this repo to replace this with something we build locally/on-the-fly as part of the e2e test and make available during the e2e?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup I'd discussed this yesterday and cc:@theishshah I'm adding the TODO and creating the issue so that we can keep track of this

Comment on lines +67 to +75
Eventually(func(g Gomega) {
err = c.Get(ctx, types.NamespacedName{Name: operator.Name}, operator)
g.Expect(err).ToNot(HaveOccurred())
g.Expect(len(operator.Status.Conditions)).To(Equal(1))
g.Expect(operator.Status.Conditions[0].Message).To(Equal("install was successful"))
}).WithTimeout(defaultTimeout).WithPolling(defaultPoll).Should(Succeed())
Copy link
Member

@joelanford joelanford Apr 27, 2023

Choose a reason for hiding this comment

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

It it necessary to pass g Gomega to the function? I would expect us to be able to do something like this:

Eventually(func() error {
    if err := doSomething(); err != nil {
		return err
	}
	return nil
}).Should(Succeed())

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right. The tests already had Eventually blocks that was passing in a Gomega object so I stuck to that, but it's not needed so removing from all of the other ones (I also want to paraphrase the tests with the right When->By->It tree structures, so that we take advantage of the ginkgo DAG test paths creation feature which becomes crucial when the test suite grows, but I'm going to tackle that in a different PR :) )

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Turns out:

 [FAILED] The function passed to Eventually had an invalid signature of func().  Functions passed to Eventually must either:

  	(a) have return values or
  	(b) take a Gomega interface as their first argument and use that Gomega instance to make assertions.

  You can learn more at https://onsi.github.io/gomega/#eventually

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I know we're used to the first approach in our existing test suites, but if we want to discuss the merits of a) vs b) we can do that in a separate PR. For now I've reverted back to using b)

Copy link
Member

@joelanford joelanford Apr 28, 2023

Choose a reason for hiding this comment

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

Oh, that function needs to return something so it can be asserted against. Updated my original comment, with a better code example.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yea that's what I meant in my previous comment, that we're using the func signature you mentioned in other test suites but changing the current approach in the code base seems like a follow up PR

client client.Client
}

func NewCatalogdEntitySource(client client.Client) *catalogdEntitySource {

Choose a reason for hiding this comment

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

Is there a compelling reason not to return input.EntitySource?

Copy link
Member

Choose a reason for hiding this comment

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

There's a go proverb "Accept interfaces, return concrete types". So my initial argument was "let's just export this", which is fine because its internal. But since its internal, its hard to care that much because we can always change it.

@anik120 anik120 force-pushed the catalogd-integration branch 4 times, most recently from b23746e to 7041aca Compare April 28, 2023 16:02
b.mu.Lock()
defer b.mu.Unlock()
if b.bundlePath == "" {
bundlePath, err := loadFromEntity[string](b.Entity, PropertyBundlePath, required)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure that this is necessary. From my testing, this particular line is what makes us have to do weird marshalling and unmarshalling logic here

I think using the loadFromEntity() function makes the most sense when you are attempting to load something you are expecting to follow a specific structure and gets unmarshalled into a struct. For this case, since we are expecting an unstructured string ("hello world!" as opposed to "{"test": "test"}") and the b.Entity.Properties is a map[string]string I think we are better off just doing something like:

Suggested change
bundlePath, err := loadFromEntity[string](b.Entity, PropertyBundlePath, required)
bundlePath, ok := b.Entity.Properties[PropertyBundlePath]

Using the above suggestion will allow us to change this to:

props["olm.bundle.path"] = bundle.Spec.Image

//
// For more details, check Reconcile and its Result here:
// - https://pkg.go.dev/sigs.k8s.io/[email protected]/pkg/reconcile
//+kubebuilder:rbac:groups=catalogd.operatorframework.io,resources=bundlemetadata,verbs=get;list;watch
Copy link
Member

@varshaprasad96 varshaprasad96 Apr 30, 2023

Choose a reason for hiding this comment

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

Just curious, why does the operator controller need to be able to watch bundlemetadata and packages? Isn't the catalog controller doing that? Are we adding watches to these objects in the operator controller's reconciler (I don't see that change in the reconciler code, just wanted to confirm it)?

Copy link
Contributor Author

@anik120 anik120 May 1, 2023

Choose a reason for hiding this comment

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

No that's a good catch, we don't actually need to watch these resources. Updated to remove the watch verb

Copy link
Member

Choose a reason for hiding this comment

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

@anik120 took another pass, I maybe wrong but do we also need get and list for those GVKs? The catalog controller is fetching bundle metadata and packages, we are getting the content through the manager's client and passing in the entities to the controller. The operator controller doesn't seem to directly interact with both the APIs (pkgs or BMD). Given that, does it need the rbac to get and list those resources?

Copy link
Member

@joelanford joelanford May 1, 2023

Choose a reason for hiding this comment

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

We need list and watch if we use the caching client, right?

Edit: one way or another, the process running the entity source needs list/watch because of the caching client in the catalogsource. My understanding is that the operator controller is the thing running the resolver and the resolver is the thing using the entity source?

Copy link
Member

Choose a reason for hiding this comment

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

We can put these rbac comments in the entity source implementation and update the rbac generator invocation to find them there if we want to keep the rules nearest the place that needs them. But that could be a follow up.

@anik120 anik120 force-pushed the catalogd-integration branch from 7b23295 to 15dcd66 Compare May 1, 2023 14:43
@@ -121,7 +115,6 @@ func (r *OperatorReconciler) reconcile(ctx context.Context, op *operatorsv1alpha
})
return ctrl.Result{}, nil
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: deletion of a blank line/whitepsace. This goes against the "minimizing" the diff, but I don't care enough.

@tmshort
Copy link
Contributor

tmshort commented May 1, 2023

Just a note that catsrc integration was added in 2973a14, this PR deletes all the added files from that commit (at least), and removes other edits of that commit.

Copy link
Contributor

@tmshort tmshort left a comment

Choose a reason for hiding this comment

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

LGTM, although so many other people reviewed this, I'd at least like to get one of them to give it 👍

Copy link
Member

@varshaprasad96 varshaprasad96 left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label May 1, 2023
Comment on lines 58 to 59
//+kubebuilder:rbac:groups=catalogd.operatorframework.io,resources=bundlemetadata,verbs=get;list
//+kubebuilder:rbac:groups=catalogd.operatorframework.io,resources=packages,verbs=get;list
Copy link
Member

Choose a reason for hiding this comment

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

Because we're using mgr.GetClient() when we setup the catalogd entity source, that means we're using the caching client. The caching client needs list and watch verbs. get isn't necessary because the caching client always gets from the cache and never actually issues get requests to the apiserver.

If you create the sample catalog and operator and then watch the logs based on this commit, you'll see lines like this:

E0505 12:53:49.867260       1 reflector.go:140] pkg/mod/k8s.io/[email protected]/tools/cache/reflector.go:169: Failed to watch *v1beta1.Package: unknown (get packages.catalogd.operatorframework.io)
E0505 12:53:50.229906       1 reflector.go:140] pkg/mod/k8s.io/[email protected]/tools/cache/reflector.go:169: Failed to watch *v1beta1.BundleMetadata: unknown (get bundlemetadata.catalogd.operatorframework.io)
E0505 12:53:52.993949       1 reflector.go:140] pkg/mod/k8s.io/[email protected]/tools/cache/reflector.go:169: Failed to watch *v1beta1.Package: unknown (get packages.catalogd.operatorframework.io)
E0505 12:53:53.228923       1 reflector.go:140] pkg/mod/k8s.io/[email protected]/tools/cache/reflector.go:169: Failed to watch *v1beta1.BundleMetadata: unknown (get bundlemetadata.catalogd.operatorframework.io)
Suggested change
//+kubebuilder:rbac:groups=catalogd.operatorframework.io,resources=bundlemetadata,verbs=get;list
//+kubebuilder:rbac:groups=catalogd.operatorframework.io,resources=packages,verbs=get;list
//+kubebuilder:rbac:groups=catalogd.operatorframework.io,resources=bundlemetadata,verbs=list;watch
//+kubebuilder:rbac:groups=catalogd.operatorframework.io,resources=packages,verbs=list;watch

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yikes. Updated.

@anik120 anik120 force-pushed the catalogd-integration branch from 15dcd66 to 35af915 Compare May 8, 2023 13:07
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label May 8, 2023
This PR replaces OLM v0 catalog operator with https://github.com/operator-framework/catalogd
as the source of entities for deppy.

Addresses operator-framework#161

Signed-off-by: Anik <[email protected]>
@anik120 anik120 force-pushed the catalogd-integration branch from 35af915 to 724f053 Compare May 8, 2023 13:18
@tmshort
Copy link
Contributor

tmshort commented May 8, 2023

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label May 8, 2023
@tmshort tmshort merged commit 5cc5cc2 into operator-framework:main May 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants