Skip to content

⚠️ Remove status.resolution - #1315

Merged
LalatenduMohanty merged 2 commits into
operator-framework:mainfrom
LalatenduMohanty:issue-1306
Oct 1, 2024
Merged

⚠️ Remove status.resolution#1315
LalatenduMohanty merged 2 commits into
operator-framework:mainfrom
LalatenduMohanty:issue-1306

Conversation

@LalatenduMohanty

@LalatenduMohantyLalatenduMohanty commented Sep 26, 2024•
edited
Loading

Copy link
Copy Markdown
Member

Fixes#1306

Description

Reviewer Checklist

  • API Go Documentation
  • Tests: Unit Tests (and E2E Tests, if appropriate)
  • Comprehensive Commit Messages
  • Links to related GitHub Issue(s)

@LalatenduMohanty
LalatenduMohanty requested a review from a team as a code ownerSeptember 26, 2024 18:57
@openshift-merge-robotopenshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 26, 2024
@netlify

netlifyBot commented Sep 26, 2024•
edited
Loading

Copy link
Copy Markdown

✅ Deploy Preview for olmv1 ready!

NameLink
🔨 Latest commit5cf7d39
🔍 Latest deploy loghttps://app.netlify.com/sites/olmv1/deploys/66fb104a086d1b00083d75ce
😎 Deploy Previewhttps://deploy-preview-1315--olmv1.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@LalatenduMohantyLalatenduMohanty changed the title ⚠️ Remove status.resolution[WIP] ⚠️ Remove status.resolutionSep 26, 2024
@openshift-ciopenshift-ciBot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 26, 2024
@openshift-merge-robotopenshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 27, 2024
@LalatenduMohanty
LalatenduMohantyforce-pushed the issue-1306 branch 6 times, most recently from e29c5e0 to 26bdc55CompareSeptember 27, 2024 19:27
@openshift-merge-robotopenshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 27, 2024
joelanford
joelanford reviewed Sep 28, 2024
View reviewed changes
Comment threadinternal/controllers/clusterextension_controller_test.go
joelanford
joelanford reviewed Sep 28, 2024
View reviewed changes
Comment threadinternal/controllers/clusterextension_controller_test.go Outdated
joelanford
joelanford reviewed Sep 28, 2024
View reviewed changes
Comment threadinternal/controllers/clusterextension_controller_test.go
require.NoError(t, cl.DeleteAllOf(ctx, &ocv1alpha1.ClusterExtension{}))
}

func verifyInvariants(ctx context.Context, t *testing.T, c client.Client, ext *ocv1alpha1.ClusterExtension) {

@joelanfordjoelanfordSep 28, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Interesting that the first test is the only place this is being called. So much for invariants if we only test them in one scenario. Haha.

I think we've decided to loosen our invariant checks, but want to call this out to make sure we talk about it.

@LalatenduMohantyLalatenduMohantySep 30, 2024

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

The lint pointed out that , these functions are not used anymore.

@LalatenduMohantyLalatenduMohanty changed the title [WIP] ⚠️ Remove status.resolutionRemove status.resolutionSep 30, 2024
@openshift-ciopenshift-ciBot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 30, 2024
joelanford
joelanford reviewed Sep 30, 2024
View reviewed changes
Comment threadtest/e2e/cluster_extension_install_test.go Outdated
@openshift-merge-robotopenshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 30, 2024
@LalatenduMohantyLalatenduMohanty changed the title Remove status.resolution⚠️ Remove status.resolutionSep 30, 2024
everettraven
everettraven reviewed Sep 30, 2024
View reviewed changes
Comment threadtest/e2e/cluster_extension_install_test.go Outdated
everettraven
everettraven reviewed Sep 30, 2024
View reviewed changes
Comment threadtest/e2e/cluster_extension_install_test.go Outdated
everettraven
everettraven reviewed Sep 30, 2024
View reviewed changes
Comment threadtest/e2e/cluster_extension_install_test.go Outdated
everettraven
everettraven reviewed Sep 30, 2024
View reviewed changes
Comment threadtest/e2e/cluster_extension_install_test.go Outdated
everettraven
everettraven reviewed Sep 30, 2024
View reviewed changes
Comment threadtest/e2e/cluster_extension_install_test.go Outdated
everettraven
everettraven previously approved these changes Sep 30, 2024
View reviewed changes

@everettraveneverettraven left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

From what I can tell this looks good!

@codecov

codecovBot commented Sep 30, 2024•
edited
Loading

Copy link
Copy Markdown

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 76.13%. Comparing base (9125118) to head (5cf7d39).
Report is 3 commits behind head on main.

Additional details and impacted files
@@ Coverage Diff @@## main #1315 +/- ##
==========================================
+ Coverage 75.99% 76.13% +0.13% 
==========================================
Files 41 41 Lines 2429 2409 -20 ==========================================
- Hits 1846 1834 -12 + Misses 410 402 -8 
Partials 173 173
FlagCoverage Δ
e2e58.32% <ø> (+0.06%)⬆️
unit52.84% <ø> (+0.10%)⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@LalatenduMohanty
LalatenduMohanty added this pull request to the merge queue Sep 30, 2024
@github-merge-queue
github-merge-queueBot removed this pull request from the merge queue due to a conflict with the base branch Sep 30, 2024
@LalatenduMohanty
LalatenduMohantyforce-pushed the issue-1306 branch 2 times, most recently from c9767f7 to e2f605cCompareSeptember 30, 2024 20:50
@LalatenduMohanty
Remove status.resolution
917303e
Fixesoperator-framework#1306
Signed-off-by: Lalatendu Mohanty <lmohanty@redhat.com>
@LalatenduMohanty
Improving the unit test code readability
5cf7d39
replacing below code with require.NotNil(ct, cond)
```
if !assert.NotNil(ct, cond) {
return
}
```
Signed-off-by: Lalatendu Mohanty <lmohanty@redhat.com>
@LalatenduMohanty
LalatenduMohanty added this pull request to the merge queue Sep 30, 2024
@github-merge-queue
github-merge-queueBot removed this pull request from the merge queue due to failed status checks Sep 30, 2024
@LalatenduMohanty
LalatenduMohanty added this pull request to the merge queue Sep 30, 2024
@LalatenduMohanty

LalatenduMohanty commented Sep 30, 2024•
edited
Loading

Copy link
Copy Markdown
MemberAuthor

e2e-kind failed for the last merge queue test : https://github.com/operator-framework/operator-controller/actions/runs/11114103134/job/30879887774

=== RUN TestClusterExtensionInstallReResolvesWhenManagedContentChanged
cluster_extension_install_test.go:643: When a cluster extension is installed from a catalog
cluster_extension_install_test.go:644: It resolves again when managed content is changed
cluster_extension_install_test.go:666: It installs the specified package with correct bundle path
cluster_extension_install_test.go:667: By creating the ClusterExtension resource
cluster_extension_install_test.go:670: By reporting a successful installation
panic: Assertion failed
goroutine 534 [running]:
github.com/stretchr/testify/assert.(*CollectT).FailNow(0x1c188e0?)
/home/runner/go/pkg/mod/github.com/stretchr/testify@v1.9.0/assert/assertions.go:1924 +0x25
github.com/stretchr/testify/require.NotNil({0x1c1df60, 0xc0000117d0}, {0x18e2620, 0x0}, {0x0, 0x0, 0x0})
/home/runner/go/pkg/mod/github.com/stretchr/testify@v1.9.0/require/require.go:1582 +0xca
github.com/operator-framework/operator-controller/test/e2e.TestClusterExtensionInstallReResolvesWhenManagedContentChanged.func1(0xc0000117d0)
/home/runner/work/operator-controller/operator-controller/test/e2e/cluster_extension_install_test.go:674 +0x153
github.com/stretchr/testify/assert.EventuallyWithT.func1()
/home/runner/go/pkg/mod/github.com/stretchr/testify@v1.9.0/assert/assertions.go:1983 +0x7f
created by github.com/stretchr/testify/assert.EventuallyWithT in goroutine 512
/home/runner/go/pkg/mod/github.com/stretchr/testify@v1.9.0/assert/assertions.go:1978 +0x277
FAIL	github.com/operator-framework/operator-controller/test/e2e	69.885s
FAIL
make: *** [Makefile:138: e2e] Error 1

@github-merge-queue
github-merge-queueBot removed this pull request from the merge queue due to failed status checks Sep 30, 2024
@LalatenduMohanty

LalatenduMohanty commented Sep 30, 2024

Copy link
Copy Markdown
MemberAuthor

This time e2e-kind passed but unit tests failed with

ok github.com/operator-framework/operator-controller/internal/contentmanager/cache	1.085s	coverage: 78.7% of statements
E0930 21:21:36.961949 21505 reflector.go:556] "Observed a panic" panic="coding error: you must register resource to list kind for every resource you're going to LIST when creating the client. See NewSimpleDynamicClientWithCustomListKinds or register the list into the scheme: /v1, Resource=pods out of map[]" stacktrace=<
goroutine 57 [running]:
k8s.io/apimachinery/pkg/util/runtime.logPanic({0x25690f8, 0x31b2f00}, {0x2029900, 0xc0004d8300})
/home/runner/go/pkg/mod/k8s.io/apimachinery@v0.31.1/pkg/util/runtime/runtime.go:107 +0xe5
k8s.io/apimachinery/pkg/util/runtime.handleCrash({0x25690f8, 0x31b2f00}, {0x2029900, 0xc0004d8300}, {0x31b2f00, 0x0, 0xc00002b858?})
/home/runner/go/pkg/mod/k8s.io/apimachinery@v0.31.1/pkg/util/runtime/runtime.go:82 +0xa2
k8s.io/apimachinery/pkg/util/runtime.HandleCrash({0x0, 0x0, 0x234f730?})
/home/runner/go/pkg/mod/k8s.io/apimachinery@v0.31.1/pkg/util/runtime/runtime.go:59 +0x18f
panic({0x2029900?, 0xc0004d8300?})
/opt/hostedtoolcache/go/1.22.5/x64/src/runtime/panic.go:770 +0x132
k8s.io/client-go/tools/cache.(*Reflector).list(0xc0002664b0, 0xc000051380)
/home/runner/go/pkg/mod/k8s.io/client-go@v0.31.1/tools/cache/reflector.go:556 +0x1493
k8s.io/client-go/tools/cache.(*Reflector).ListAndWatch(0xc0002664b0, 0xc000051380)
/home/runner/go/pkg/mod/k8s.io/client-go@v0.31.1/tools/cache/reflector.go:362 +0x3d1
k8s.io/client-go/tools/cache.(*Reflector).Run.func1()
/home/runner/go/pkg/mod/k8s.io/client-go@v0.31.1/tools/cache/reflector.go:307 +0x3d
k8s.io/apimachinery/pkg/util/wait.BackoffUntil.func1(0xc000088ee8)
/home/runner/go/pkg/mod/k8s.io/apimachinery@v0.31.1/pkg/util/wait/backoff.go:226 +0x42
k8s.io/apimachinery/pkg/util/wait.BackoffUntil(0xc00002bee8, {0x2551b60, 0xc00013e050}, 0x1, 0xc000051380)
/home/runner/go/pkg/mod/k8s.io/apimachinery@v0.31.1/pkg/util/wait/backoff.go:227 +0xc5
k8s.io/client-go/tools/cache.(*Reflector).Run(0xc0002664b0, 0xc000051380)
/home/runner/go/pkg/mod/k8s.io/client-go@v0.31.1/tools/cache/reflector.go:306 +0x2f6
k8s.io/client-go/tools/cache.(*controller).Run.(*Group).StartWithChannel.func2()
/home/runner/go/pkg/mod/k8s.io/apimachinery@v0.31.1/pkg/util/wait/wait.go:55 +0x39
k8s.io/apimachinery/pkg/util/wait.(*Group).Start.func1()
/home/runner/go/pkg/mod/k8s.io/apimachinery@v0.31.1/pkg/util/wait/wait.go:72 +0x87
created by k8s.io/apimachinery/pkg/util/wait.(*Group).Start in goroutine 66
/home/runner/go/pkg/mod/k8s.io/apimachinery@v0.31.1/pkg/util/wait/wait.go:70 +0xe5
>
panic: coding error: you must register resource to list kind for every resource you're going to LIST when creating the client. See NewSimpleDynamicClientWithCustomListKinds or register the list into the scheme: /v1, Resource=pods out of map[] [recovered]
panic: coding error: you must register resource to list kind for every resource you're going to LIST when creating the client. See NewSimpleDynamicClientWithCustomListKinds or register the list into the scheme: /v1, Resource=pods out of map[]
goroutine 57 [running]:
k8s.io/apimachinery/pkg/util/runtime.handleCrash({0x25690f8, 0x31b2f00}, {0x2029900, 0xc0004d8300}, {0x31b2f00, 0x0, 0xc00002b858?})
/home/runner/go/pkg/mod/k8s.io/apimachinery@v0.31.1/pkg/util/runtime/runtime.go:89 +0x148
k8s.io/apimachinery/pkg/util/runtime.HandleCrash({0x0, 0x0, 0x234f730?})
/home/runner/go/pkg/mod/k8s.io/apimachinery@v0.31.1/pkg/util/runtime/runtime.go:59 +0x18f
panic({0x2029900?, 0xc0004d8300?})
/opt/hostedtoolcache/go/1.22.5/x64/src/runtime/panic.go:770 +0x132
k8s.io/client-go/tools/cache.(*Reflector).list(0xc0002664b0, 0xc000051380)
/home/runner/go/pkg/mod/k8s.io/client-go@v0.31.1/tools/cache/reflector.go:556 +0x1493
k8s.io/client-go/tools/cache.(*Reflector).ListAndWatch(0xc0002664b0, 0xc000051380)
/home/runner/go/pkg/mod/k8s.io/client-go@v0.31.1/tools/cache/reflector.go:362 +0x3d1
k8s.io/client-go/tools/cache.(*Reflector).Run.func1()
/home/runner/go/pkg/mod/k8s.io/client-go@v0.31.1/tools/cache/reflector.go:307 +0x3d
k8s.io/apimachinery/pkg/util/wait.BackoffUntil.func1(0xc000088ee8)
/home/runner/go/pkg/mod/k8s.io/apimachinery@v0.31.1/pkg/util/wait/backoff.go:226 +0x42
k8s.io/apimachinery/pkg/util/wait.BackoffUntil(0xc00002bee8, {0x2551b60, 0xc00013e050}, 0x1, 0xc000051380)
/home/runner/go/pkg/mod/k8s.io/apimachinery@v0.31.1/pkg/util/wait/backoff.go:227 +0xc5
k8s.io/client-go/tools/cache.(*Reflector).Run(0xc0002664b0, 0xc000051380)
/home/runner/go/pkg/mod/k8s.io/client-go@v0.31.1/tools/cache/reflector.go:306 +0x2f6
k8s.io/client-go/tools/cache.(*controller).Run.(*Group).StartWithChannel.func2()
/home/runner/go/pkg/mod/k8s.io/apimachinery@v0.31.1/pkg/util/wait/wait.go:55 +0x39
k8s.io/apimachinery/pkg/util/wait.(*Group).Start.func1()
/home/runner/go/pkg/mod/k8s.io/apimachinery@v0.31.1/pkg/util/wait/wait.go:72 +0x87
created by k8s.io/apimachinery/pkg/util/wait.(*Group).Start in goroutine 66
/home/runner/go/pkg/mod/k8s.io/apimachinery@v0.31.1/pkg/util/wait/wait.go:70 +0xe5
FAIL	github.com/operator-framework/operator-controller/internal/contentmanager/source	0.114s

@LalatenduMohanty
LalatenduMohanty added this pull request to the merge queue Oct 1, 2024
@LalatenduMohanty

LalatenduMohanty commented Oct 1, 2024•
edited
Loading

Copy link
Copy Markdown
MemberAuthor

#1323 is merged to fix the unit test race condition.

Merged via the queue into operator-framework:main with commit fba8473Oct 1, 2024
@LalatenduMohanty
LalatenduMohanty deleted the issue-1306 branch October 1, 2024 10:36
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@joelanfordjoelanfordjoelanford left review comments
+1 more reviewer
@everettraveneverettraveneverettraven approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Remove .status.resolution field

4 participants

@LalatenduMohanty@joelanford@everettraven@openshift-merge-robot