Use catalogd HTTP Server instead of `CatalogMetadata` API #411

everettraven · 2023-09-14T17:58:17Z

Description

Bumps catalogd dependency to v0.7.0
Update the internal/catalogmetadata package to make HTTP requests to the catalogd HTTP server that serves catalog contents instead of using the deprecated CatalogMetadata API
Minimally update the internal/resolution/entitysources/catalogdsource.go implementation to also make HTTP requests (didn't do any optimization work here since it is planned to be removed by Remove Entities and EntitySources #413)
resolves Update catalog content retrieval method #293

Reviewer Checklist

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

codecov · 2023-09-14T18:02:40Z

Codecov Report

Patch coverage: 80.82% and project coverage change: -7.66% ⚠️

Comparison is base (52a1e28) 84.42% compared to head (e0aede3) 76.77%.
Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #411      +/-   ##
==========================================
- Coverage   84.42%   76.77%   -7.66%     
==========================================
  Files          23       21       -2     
  Lines         777      788      +11     
==========================================
- Hits          656      605      -51     
- Misses         86      148      +62     
  Partials       35       35

Flag	Coverage Δ
e2e	`?`
unit	`76.77% <80.82%> (+0.08%)`	⬆️

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

Files Changed	Coverage Δ
internal/catalogmetadata/cache/cache.go	`70.21% <70.21%> (ø)`
internal/catalogmetadata/client/client.go	`96.07% <100.00%> (+6.60%)`	⬆️

... and 6 files with indirect coverage changes

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

internal/catalogmetadata/client/client.go

internal/catalogmetadata/client/client_test.go

internal/catalogmetadata/unmarshal.go

everettraven · 2023-09-18T17:32:47Z

IIUC the plan is to hold this PR on a catalogd release that contains operator-framework/catalogd#168 and that caching logic should be added. Due to this I will be placing a hold on this PR

/hold

ncdc · 2023-09-18T18:02:54Z

@everettraven please mark it as a draft PR - we don't use Prow on this repo. Thanks!

internal/catalogmetadata/client/client.go

internal/catalogmetadata/client/client_test.go

internal/resolution/entitysources/catalogdsource.go

stevekuznetsov · 2023-09-19T12:45:31Z

Do I read correctly that now we're caching the full contents of every catalog in memory?

everettraven · 2023-09-19T12:56:59Z

Do I read correctly that now we're caching the full contents of every catalog in memory?

Correct, but I'm actively working on a different caching mechanism that will use the local filesystem for caching the contents and in-memory cache for some additional information that won't exist in the cached file.

joelanford · 2023-09-19T14:08:13Z

Do I read correctly that now we're caching the full contents of every catalog in memory?

Technically, we are doing that on main already because we have an informer cache for CatalogMetadata.

internal/catalogmetadata/cache/cache.go

test/operator-framework-e2e/operator_framework_test.go

internal/catalogmetadata/client/client_test.go

internal/catalogmetadata/cache/cache.go

m1kola · 2023-09-21T13:43:01Z

internal/catalogmetadata/client/client.go

+	FetchCatalogContents(ctx context.Context, catalog *catalogd.Catalog) (io.ReadCloser, error)
+}
+
+func New(cl client.Client, fetcher Fetcher) *Client {


I called this thing "client" due to lack of imagination on my side. Don't hesitate to rename, if you feel like it & have a better name in mind.

everettraven · 2023-09-21T14:34:26Z

Looks like actions are hung up - closing and re-opening to kick them

internal/catalogmetadata/cache/cache.go

internal/catalogmetadata/client/client_test.go

m1kola

I think it looks good to me, but I haven't looked into tests just yet. Hoping to come back to this tomorrow.

cmd/manager/main.go

internal/catalogmetadata/cache/cache.go

everettraven · 2023-09-21T17:00:03Z

/hold cancel

joelanford · 2023-09-21T17:32:46Z

internal/catalogmetadata/cache/cache_test.go

+					rc, err := c.FetchCatalogContents(ctx, tst.catalog)
+					assert.NoError(t, err)
+					defer rc.Close()
+					data, err := io.ReadAll(rc)
+					assert.NoError(t, err)
+					assert.Equal(t, tst.contents, data)


I can see how these extra tests provide coverage of the cache code, but unless I'm missing something, they don't actually verify whether cached data or fetched data is returned.

You're correct on this one - I missed forcing an update to the contents returned by the mock http.RoundTripper. The test below verifies this by ensuring the contents returned by the function are the same as the contents that should be returned by the http.RoundTripper

For more clarity, the logic behind this is:

If tst.tripper.content is changed and the cache is not used the content that will be returned != tst.contents

If tst.tripper.content is changed and the status.resolvedSource.image.ref is changed it won't hit the cache and thus the response should be different than original contents and instead match what the tst.tripper is told to return.

(I've updated the code a bit to try and make this more clear when reading it)

joelanford · 2023-09-21T17:37:01Z

internal/catalogmetadata/cache/cache_test.go

+			contents   []byte
+			wantErr    bool
+			tripper    *MockTripper
+			extraTests func(t *testing.T, c client.Fetcher, ctx context.Context, tst test)


I've not seen this pattern before, and I'm worried it might be confusing, ripe for misuse later, or become an abstraction that is no longer useful and that we keep carrying because it's here.

WDYT of factoring these into tests into separate Test* functions or t.Run blocks that just do a series of things in order?

TLDR: I'd prefer this be done in a follow-up if acceptable since this follows the table driven testing the rest of the catalogmetadata package seems to follow for multiple tests of the same method.

The thinking here was that this would be a function signature used to run a few extra validation/test steps and at it's base, all of these test cases have to go through the same initial run. The idea was to reduce code repetition while still achieving the ability to do some additional testing steps for the, currently, 2 tests that need it.

I'd prefer to keep this as is for now since it follows the same table driven testing pattern that exists in the rest of the catalogmetadata package and reduces code repetition.

I'm not opposed to table-driven, but that sort of hits on my concern. This isn't totally table-driven because the table actually contains more test code rather then being more declarative/static test input/expectations.

If I'm the only one with this concern, I won't hold this PR up on it though.

That's a fair point - I'm still new to the table driven testing pattern so I wasn't aware this was actually an anti-pattern for it. Thanks for pointing it out, til :)

Changed it in 76c9cc8 - let me know if this addresses your concerns

internal/catalogmetadata/client/client_test.go

internal/catalogmetadata/cache/cache_test.go

internal/catalogmetadata/client/client_test.go

joelanford

One more nit I mentioned above, but looks good. Thanks for sticking with it!

Signed-off-by: Bryce Palmer <[email protected]>

We ended up pulling in the status.contentURL feature from catalogd to resolve the related suggestion.

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 14, 2023

joelanford reviewed Sep 14, 2023

View reviewed changes

internal/catalogmetadata/client/client.go Outdated Show resolved Hide resolved

everettraven mentioned this pull request Sep 14, 2023

Bump github.com/operator-framework/catalogd from 0.5.0 to 0.6.0 #410

Merged

everettraven force-pushed the feature/catalogd-httpserver branch 2 times, most recently from e94fddd to f70e35b Compare September 14, 2023 19:42

everettraven changed the title ~~WIP: Use catalogd HTTP Server instead of CatalogMetadata API~~ Use catalogd HTTP Server instead of CatalogMetadata API Sep 14, 2023

everettraven marked this pull request as ready for review September 14, 2023 19:42

everettraven requested a review from a team as a code owner September 14, 2023 19:42

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 14, 2023

everettraven mentioned this pull request Sep 15, 2023

Follow-Up: Investigate caching for interactions with catalogd's HTTP server #415

Closed

grokspawn approved these changes Sep 15, 2023

View reviewed changes

anik120 previously requested changes Sep 15, 2023

View reviewed changes

internal/catalogmetadata/client/client.go Outdated Show resolved Hide resolved

m1kola reviewed Sep 18, 2023

View reviewed changes

openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 18, 2023

everettraven marked this pull request as draft September 18, 2023 18:04

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 18, 2023

stevekuznetsov reviewed Sep 19, 2023

View reviewed changes

everettraven mentioned this pull request Sep 19, 2023

Improve resolution efficiency #418

Closed

m1kola mentioned this pull request Sep 20, 2023

Remove Entities and EntitySources #413

Merged

stevekuznetsov reviewed Sep 20, 2023

View reviewed changes

internal/catalogmetadata/cache/cache.go Outdated Show resolved Hide resolved

stevekuznetsov reviewed Sep 20, 2023

View reviewed changes

internal/catalogmetadata/cache/cache.go Outdated Show resolved Hide resolved

stevekuznetsov reviewed Sep 20, 2023

View reviewed changes

internal/catalogmetadata/cache/cache.go Outdated Show resolved Hide resolved

stevekuznetsov reviewed Sep 20, 2023

View reviewed changes

internal/catalogmetadata/cache/cache.go Outdated Show resolved Hide resolved

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 20, 2023

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 21, 2023

m1kola reviewed Sep 21, 2023

View reviewed changes

everettraven force-pushed the feature/catalogd-httpserver branch from 1ae9b4a to fcb56fd Compare September 21, 2023 14:04

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 21, 2023

everettraven mentioned this pull request Sep 21, 2023

Follow-Up for #411: Should unavailable Catalogs be ignored during resolution? #428

Open

everettraven closed this Sep 21, 2023

everettraven reopened this Sep 21, 2023

everettraven force-pushed the feature/catalogd-httpserver branch from fcb56fd to 038505c Compare September 21, 2023 14:36

m1kola reviewed Sep 21, 2023

View reviewed changes

internal/catalogmetadata/cache/cache.go Outdated Show resolved Hide resolved

internal/catalogmetadata/client/client_test.go Show resolved Hide resolved

everettraven force-pushed the feature/catalogd-httpserver branch from 038505c to 5411ba6 Compare September 21, 2023 15:13

m1kola reviewed Sep 21, 2023

View reviewed changes

joelanford reviewed Sep 21, 2023

View reviewed changes

cmd/manager/main.go Outdated Show resolved Hide resolved

joelanford reviewed Sep 21, 2023

View reviewed changes

internal/catalogmetadata/cache/cache.go Show resolved Hide resolved

everettraven force-pushed the feature/catalogd-httpserver branch from 5411ba6 to 1427f79 Compare September 21, 2023 16:26

openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 21, 2023

joelanford reviewed Sep 21, 2023

View reviewed changes

internal/catalogmetadata/client/client_test.go Outdated Show resolved Hide resolved

joelanford reviewed Sep 21, 2023

View reviewed changes

internal/catalogmetadata/cache/cache_test.go Outdated Show resolved Hide resolved

joelanford reviewed Sep 21, 2023

View reviewed changes

internal/catalogmetadata/client/client_test.go Outdated Show resolved Hide resolved

joelanford approved these changes Sep 21, 2023

View reviewed changes

use catalogd HTTP server instead of CatalogMetadata API

e0aede3

Signed-off-by: Bryce Palmer <[email protected]>

everettraven force-pushed the feature/catalogd-httpserver branch from 76c9cc8 to e0aede3 Compare September 21, 2023 19:41

m1kola mentioned this pull request Sep 22, 2023

Bump github.com/operator-framework/catalogd from 0.6.0 to 0.7.0 #429

Closed

joelanford added this pull request to the merge queue Sep 22, 2023

Merged via the queue into operator-framework:main with commit 05ffd85 Sep 22, 2023
10 of 12 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use catalogd HTTP Server instead of `CatalogMetadata` API #411

Use catalogd HTTP Server instead of `CatalogMetadata` API #411

everettraven commented Sep 14, 2023 •

edited

Loading

codecov bot commented Sep 14, 2023 •

edited

Loading

everettraven commented Sep 18, 2023

ncdc commented Sep 18, 2023

stevekuznetsov commented Sep 19, 2023

everettraven commented Sep 19, 2023

joelanford commented Sep 19, 2023

m1kola Sep 21, 2023

everettraven commented Sep 21, 2023

m1kola left a comment

everettraven commented Sep 21, 2023

joelanford Sep 21, 2023

everettraven Sep 21, 2023

everettraven Sep 21, 2023

everettraven Sep 21, 2023

joelanford Sep 21, 2023

everettraven Sep 21, 2023

joelanford Sep 21, 2023

joelanford Sep 21, 2023

everettraven Sep 21, 2023

joelanford left a comment •

edited

Loading

Use catalogd HTTP Server instead of CatalogMetadata API #411

Use catalogd HTTP Server instead of CatalogMetadata API #411

Conversation

everettraven commented Sep 14, 2023 • edited Loading

Description

Reviewer Checklist

codecov bot commented Sep 14, 2023 • edited Loading

Codecov Report

everettraven commented Sep 18, 2023

ncdc commented Sep 18, 2023

stevekuznetsov commented Sep 19, 2023

everettraven commented Sep 19, 2023

joelanford commented Sep 19, 2023

Choose a reason for hiding this comment

everettraven commented Sep 21, 2023

m1kola left a comment

Choose a reason for hiding this comment

everettraven commented Sep 21, 2023

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

joelanford left a comment • edited Loading

Choose a reason for hiding this comment

Use catalogd HTTP Server instead of `CatalogMetadata` API #411

Use catalogd HTTP Server instead of `CatalogMetadata` API #411

everettraven commented Sep 14, 2023 •

edited

Loading

codecov bot commented Sep 14, 2023 •

edited

Loading

joelanford left a comment •

edited

Loading