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.

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

(bugfix): add label selector to sync list queries #112

Merged
merged 7 commits into from
Jul 18, 2023

Conversation

everettraven
Copy link
Collaborator

@everettraven everettraven commented Jul 13, 2023

prevents overwriting of resources not belonging to the Catalog currently being reconciled

closes #106

to prevent overwriting of resources not belonging to the Catalog
currently being reconciled

Signed-off-by: Bryce Palmer <bpalmer@redhat.com>
@codecov
Copy link

codecov bot commented Jul 13, 2023

Codecov Report

Merging #112 (be0e2ca) into main (3b1780e) will increase coverage by 1.50%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##             main     #112      +/-   ##
==========================================
+ Coverage   78.11%   79.62%   +1.50%     
==========================================
  Files           1        1              
  Lines         265      265              
==========================================
+ Hits          207      211       +4     
+ Misses         37       33       -4     
  Partials       21       21              
Impacted Files Coverage Δ
pkg/controllers/core/catalog_controller.go 79.62% <0.00%> (+1.50%) ⬆️

@joelanford
Copy link
Member

WDYT about adding/updating tests to cover this and make sure we don't regress this behavior?

and update them slightly as it seemed like the feature gates
were not appropriately enabled resulting in tests not running

Signed-off-by: Bryce Palmer <bpalmer@redhat.com>
Expect(res).To(Equal(ctrl.Result{}))
Expect(err).ToNot(HaveOccurred())

expectedCm := &v1alpha1.CatalogMetadata{}
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if directly creating the Package/BundleMetadata/CatalogMetadata, running the other catalog's reconcile, and then checking that this still exists is brittle? If we change the label selector we use we'd invalidate this test.

What about instead, just creating two different Catalog objects (they can ref the same image), reconcile both of them, and then check that all expected Package/BundleMetadata/CatalogMetadata are present?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah, that's a good point - I'll take a look into this

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done across a couple commits but should be good to go

Signed-off-by: Bryce Palmer <bpalmer@redhat.com>
instead of creating an indivdual resource.

Signed-off-by: Bryce Palmer <bpalmer@redhat.com>
Signed-off-by: Bryce Palmer <bpalmer@redhat.com>
Signed-off-by: Bryce Palmer <bpalmer@redhat.com>
Signed-off-by: Bryce Palmer <bpalmer@redhat.com>
Copy link
Collaborator

@anik120 anik120 left a comment

Choose a reason for hiding this comment

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

lgtm

@everettraven everettraven merged commit 7a98e45 into operator-framework:main Jul 18, 2023
7 of 8 checks passed
@everettraven
Copy link
Collaborator Author

/cherrypick release-v0.4

@openshift-cherrypick-robot

@everettraven: new pull request created: #123

In response to this:

/cherrypick release-v0.4

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

catalogd doesn't support multiple catalog
5 participants