⚠️ change catalog-specific URL from full path to API endpoint ref#429
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #429 +/- ##
==========================================
- Coverage 38.36% 37.83% -0.54%
==========================================
Files 15 15
Lines 941 1208 +267
==========================================
+ Hits 361 457 +96
- Misses 530 701 +171
Partials 50 50 ☔ View full report in Codecov by Sentry. |
4fd529a to
86c886c
Compare
|
e2e/upgrade-e2e will not work right because this is a breaking API change and the test pulls the latest release tag. Until this PR merges and we release a new tag, this test will not pass. |
eebf82e to
5ee9eaa
Compare
|
Setting this WIP until after the RFC completes review, but it's ready for eyeballs so I don't think it belongs in a draft state. |
fcd19bb to
3686929
Compare
|
to
Edit: a few of us met yesterday to discuss in depth, and we determined that the catalog-centric API approach was more conducive to near-future plans to add digest-level information. So we're sticking with the catalog-centric API. |
926262f to
c0395e6
Compare
c0395e6 to
6ba8c8d
Compare
6ba8c8d to
79c2c59
Compare
|
Thanks again for the review passes folks! |
d4d176d to
ea1ea0e
Compare
ea1ea0e to
6acdf9b
Compare
everettraven
left a comment
There was a problem hiding this comment.
One minor thing in the readme. Other than that LGTM
Signed-off-by: Jordan Keister <jordan@nimblewidget.com>
6acdf9b to
28e5fbb
Compare
| // can read the content of a catalog | ||
| // urls contains the URLs that can be used to access the catalog. | ||
| // +optional | ||
| ContentURL string `json:"contentURL,omitempty"` |
There was a problem hiding this comment.
You should add a "tombstone" representing this API field now that it has been removed, to prevent it being re-added as a different serialisation
If contentURL in the future was re-added as a struct, instead of string, this would break clients who expect to be able to unmarshal it as a string
There was a problem hiding this comment.
We have discussed this and the general consensus is that we are not going to fix it as we have not released GA APIs upstream. Also we do not have many clients who uses OLM V1 (indirectly this API) . We have been communicating that these set of APIs are not stable till we release V1 GA APIs.
change catalog-specific URL from full path to API endpoint ref
solves #427 and implements phase 1 of the CatalogD expandable interface.
This phase just moves from
status.contentURLtostatus.urls.base, and will be used to provide reference to the catalog-specific API instead of the full path to JSONLines-formatted content, plus tests and documentation.Signed-off-by: Jordan Keister jordan@nimblewidget.com