-
Notifications
You must be signed in to change notification settings - Fork 31
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
⚠️ 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
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 minor thing in the readme. Other than that LGTM
Signed-off-by: Jordan Keister <jordan@nimblewidget.com>
6acdf9b
to
28e5fbb
Compare
d70f01f
// +optional | ||
ContentURL string `json:"contentURL,omitempty"` |
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 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.contentURL
tostatus.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