-
Notifications
You must be signed in to change notification settings - Fork 541
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
Bug 1823714: Update PkgManifest upon catsrc update #1482
Bug 1823714: Update PkgManifest upon catsrc update #1482
Conversation
@Bowenislandsong: This pull request references Bugzilla bug 1823714, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker. 3 validation(s) were run on this bug
In response to this:
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. |
961db39
to
006c12b
Compare
/retest |
1 similar comment
/retest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @Bowenislandsong - nice work, this looks good. I have a few questions that I'd like you to address before I LGTM. Thanks!
@@ -189,6 +189,15 @@ func (p *RegistryProvider) syncCatalogSource(obj interface{}) (syncError error) | |||
Namespace: source.GetNamespace(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You commit message has misspelled words: triggred
andaddes
. You should consider enabling spellcheck in VIM or whichever editor you use to create your commit messages.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
neat! did not know that's an option
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
funny, i went and looked. turns out i had it on. Somehow i dont see notifiations. I'll check what was wrong with that.
|
||
logger.Infof("updating PackageManifest based on CatalogSource changes: %v", key) | ||
timeout, cancel := context.WithTimeout(context.Background(), cacheTimeout) | ||
defer cancel() | ||
client, err := p.registryClient(key) | ||
if err == nil { | ||
err = p.refreshCache(timeout, client) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see that your adding this code to check if the list of packages has changed. The code looks correct in that it will update the list of packagemanifests but I have two questions:
- Could you make sure that the error is captured when the PackageServer is unable to refresh the cache? Either as a log or returning the error?
- It looks like this could be moved into the if statement below to prevent it from being called when sources don't already exist. Could you investigate?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- wow that was a very nice catch, I assumed the return was err instead of syncError. Kinda a weird way to express dislike for this kind of error return method.
- good call, that would reduce the traffic. I somehow thought about it backward initially.
test/e2e/packagemanifest_e2e_test.go
Outdated
@@ -168,6 +169,74 @@ var _ = Describe("Package Manifest", func() { | |||
require.True(GinkgoT(), ok, "Expect this image %s to exist in the related images list\n", v) | |||
} | |||
}) | |||
It("updating CatalogSource", func() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could the test be more structured to use ginkgo's container blocks and replacing assertions with gomega matcher library? You could refer to #1424 as a reference
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you could refer to this cheatsheet
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right, that would be much better. I did think about it, but may I suggest that we do that in a separate PR and have this test arranged exactly like the other tests in this file? IMO refactoring should be its own PR rather than smuggles in bug fixes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I updated the test to at least not to use assert and conform to the other tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a new test -- why create more future work for ourselves? If this bug were on fire, I can see the argument for addressing the test feedback in a follow-up, but it doesn't seem to be urgent enough to do that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lets wait for https://github.com/operator-framework/operator-lifecycle-manager/pull/1439/files to merge first.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with @benluddy
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I agree, but I am saying waiting for https://github.com/operator-framework/operator-lifecycle-manager/pull/1439/files will resolve all of our concerns. That PR is very close to merging.
/hold
482de5c
to
a8b8e85
Compare
a8b8e85
to
4ea1de0
Compare
/test e2e-aws-olm |
/retest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
one or two comments.
client, err := p.registryClient(key) | ||
if err != nil { | ||
syncError = err | ||
return |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
client, err := p.registryClient(key) | |
if err != nil { | |
syncError = err | |
return | |
client, syncError := p.registryClient(key) | |
if syncErr != nil { | |
return | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! I will update once the dependent PR is merged!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just realized, i can't do this, and was delieberatly avoiding it. This reinitialized syncError
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we just return err
on line 199?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Bowenislandsong just change :=
to =
and decalre the client variable before the function call
e2719ca
to
3224d0c
Compare
/hold cancel |
675ebe7
to
a665578
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. I just had one comment for the e2e test.
36faf24
to
dc87998
Compare
/retest |
/retest e2e-aws-olm |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few non-blocking nits, nice work.
client, err := p.registryClient(key) | ||
if err != nil { | ||
syncError = err | ||
return |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Bowenislandsong just change :=
to =
and decalre the client variable before the function call
test/e2e/packagemanifest_e2e_test.go
Outdated
catalogSource, err = crc.OperatorsV1alpha1().CatalogSources(testNamespace).Get(context.TODO(), catalogSource.GetName(), metav1.GetOptions{}) | ||
Expect(err).NotTo(HaveOccurred(), "error getting catalogSource") | ||
|
||
displayName := "updated Name" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't this be =
not :=
c93b402
to
d874c35
Compare
/lgtm |
d874c35
to
00abce0
Compare
The catsrc update sync triggered by the informer does not update pkgmanifest. This commit adds an update mechanism.
a63c3ad
to
156f12b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
/approve |
1 similar comment
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: awgreene, Bowenislandsong, dinhxuanvu, 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:
Approvers can indicate their approval by writing |
/retest Please review the full test history for this PR and help us cut down flakes. |
2 similar comments
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest |
/retest Please review the full test history for this PR and help us cut down flakes. |
2 similar comments
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
@Bowenislandsong: All pull requests linked via external trackers have merged: operator-framework/operator-lifecycle-manager#1482. Bugzilla bug 1823714 has been moved to the MODIFIED state. In response to this:
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. |
@Bowenislandsong: #1482 failed to apply on top of branch "release-4.3":
In response to this:
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. |
@Bowenislandsong: #1482 failed to apply on top of branch "release-4.2":
In response to this:
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. |
@Bowenislandsong: #1482 failed to apply on top of branch "release-4.4":
In response to this:
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. |
The catsrc update sync triggred by the informer does not update pkgmanifest. This commit addes update mechanism.
Description of the change:
Motivation for the change:
Reviewer Checklist
/docs