Skip to content
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.

Sign up for GitHub

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

Jump to bottom

deprecation schema & import/render support #1151

Merged
merged 1 commit into from
Nov 6, 2023
Merged

deprecation schema & import/render support #1151

merged 1 commit into from
Nov 6, 2023

Conversation

grokspawn
Copy link
Contributor

@grokspawn grokspawn commented Oct 24, 2023 •
edited
Loading

parent tracker
Solves #1150

Description of the change:

Motivation for the change:

Reviewer Checklist

  • Implementation matches the proposed design, or proposal is updated to match implementation
  • Sufficient unit test coverage
  • Sufficient end-to-end test coverage
  • Docs updated or added to /docs
  • Commit messages sensible and descriptive

@openshift-ci 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 Oct 24, 2023
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Oct 24, 2023

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 24, 2023
@grokspawn grokspawn changed the title poc for schema import poc for deprecation schema import Oct 24, 2023
@codecov
Copy link

codecov bot commented Oct 24, 2023 •
edited
Loading

Codecov Report

Attention: 13 lines in your changes are missing coverage. Please review.

Comparison is base (b137480) 53.81% compared to head (fd2c236) 53.80%.
Report is 1 commits behind head on master.

Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #1151      +/-   ##
==========================================
- Coverage   53.81%   53.80%   -0.01%     
==========================================
  Files         108      108              
  Lines       10224    10237      +13     
==========================================
+ Hits         5502     5508       +6     
- Misses       3749     3751       +2     
- Partials      973      978       +5
Files Coverage Δ
alpha/action/render.go 66.94% <100.00%> (-0.41%) ⬇️
alpha/declcfg/declcfg.go 82.60% <100.00%> (+2.12%) ⬆️
alpha/template/semver/semver.go 69.19% <0.00%> (+0.86%) ⬆️
alpha/declcfg/load.go 76.82% <66.66%> (-0.95%) ⬇️
alpha/declcfg/write.go 66.79% <16.66%> (-2.47%) ⬇️

... and 2 files with indirect coverage changes

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

@grokspawn grokspawn removed the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 24, 2023
@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 26, 2023
stevekuznetsov
stevekuznetsov reviewed Oct 27, 2023
View reviewed changes
alpha/declcfg/load_test.go Outdated
assertion: require.NoError,
expected: &DeclarativeConfig{
Packages: []Package{
{Schema: "olm.package", Name: "cockroachdb", DefaultChannel: "stable-5.x", Icon: &Icon{Data: []uint8{0x3c, 0x73, 0x76, 0x67, 0x20, 0x78, 0x6d, 0x6c, 0x6e, 0x73, 0x3d, 0x22, 0x68, 0x74, 0x74, 0x70, 0x3a, 0x2f, 0x2f, 0x77, 0x77, 0x77, 0x2e, 0x77, 0x33, 0x2e, 0x6f, 0x72, 0x67, 0x2f, 0x32, 0x30, 0x30, 0x30, 0x2f, 0x73, 0x76, 0x67, 0x22, 0x20, 0x76, 0x69, 0x65, 0x77, 0x42, 0x6f, 0x78, 0x3d, 0x22, 0x30, 0x20, 0x30, 0x20, 0x33, 0x31, 0x2e, 0x38, 0x32, 0x20, 0x33, 0x32, 0x22, 0x20, 0x77, 0x69, 0x64, 0x74, 0x68, 0x3d, 0x22, 0x32, 0x34, 0x38, 0x36, 0x22, 0x20, 0x68, 0x65, 0x69, 0x67, 0x68, 0x74, 0x3d, 0x22, 0x32, 0x35, 0x30, 0x30, 0x22, 0x3e, 0x3c, 0x74, 0x69, 0x74, 0x6c, 0x65, 0x3e, 0x43, 0x4c, 0x3c, 0x2f, 0x74, 0x69, 0x74, 0x6c, 0x65, 0x3e, 0x3c, 0x70, 0x61, 0x74, 0x68, 0x20, 0x64, 0x3d, 0x22, 0x4d, 0x31, 0x39, 0x2e, 0x34, 0x32, 0x20, 0x39, 0x2e, 0x31, 0x37, 0x61, 0x31, 0x35, 0x2e, 0x33, 0x39, 0x20, 0x31, 0x35, 0x2e, 0x33, 0x39, 0x20, 0x30, 0x20, 0x30, 0x20, 0x31, 0x2d, 0x33, 0x2e, 0x35, 0x31, 0x2e, 0x34, 0x20, 0x31, 0x35, 0x2e, 0x34, 0x36, 0x20, 0x31, 0x35, 0x2e, 0x34, 0x36, 0x20, 0x30, 0x20, 0x30, 0x20, 0x31, 0x2d, 0x33, 0x2e, 0x35, 0x31, 0x2d, 0x2e, 0x34, 0x20, 0x31, 0x35, 0x2e, 0x36, 0x33, 0x20, 0x31, 0x35, 0x2e, 0x36, 0x33, 0x20, 0x30, 0x20, 0x30, 0x20, 0x31, 0x20, 0x33, 0x2e, 0x35, 0x31, 0x2d, 0x33, 0x2e, 0x39, 0x31, 0x20, 0x31, 0x35, 0x2e, 0x37, 0x31, 0x20, 0x31, 0x35, 0x2e, 0x37, 0x31, 0x20, 0x30, 0x20, 0x30, 0x20, 0x31, 0x20, 0x33, 0x2e, 0x35, 0x31, 0x20, 0x33, 0x2e, 0x39, 0x31, 0x7a, 0x4d, 0x33, 0x30, 0x20, 0x2e, 0x35, 0x37, 0x41, 0x31, 0x37, 0x2e, 0x32, 0x32, 0x20, 0x31, 0x37, 0x2e, 0x32, 0x32, 0x20, 0x30, 0x20, 0x30, 0x20, 0x30, 0x20, 0x32, 0x35, 0x2e, 0x35, 0x39, 0x20, 0x30, 0x61, 0x31, 0x37, 0x2e, 0x34, 0x20, 0x31, 0x37, 0x2e, 0x34, 0x20, 0x30, 0x20, 0x30, 0x20, 0x30, 0x2d, 0x39, 0x2e, 0x36, 0x38, 0x20, 0x32, 0x2e, 0x39, 0x33, 0x41, 0x31, 0x37, 0x2e, 0x33, 0x38, 0x20, 0x31, 0x37, 0x2e, 0x33, 0x38, 0x20, 0x30, 0x20, 0x30, 0x20, 0x30, 0x20, 0x36, 0x2e, 0x32, 0x33, 0x20, 0x30, 0x61, 0x31, 0x37, 0x2e, 0x32, 0x32, 0x20, 0x31, 0x37, 0x2e, 0x32, 0x32, 0x20, 0x30, 0x20, 0x30, 0x20, 0x30, 0x2d, 0x34, 0x2e, 0x34, 0x34, 0x2e, 0x35, 0x37, 0x41, 0x31, 0x36, 0x2e, 0x32, 0x32, 0x20, 0x31, 0x36, 0x2e, 0x32, 0x32, 0x20, 0x30, 0x20, 0x30, 0x20, 0x30, 0x20, 0x30, 0x20, 0x31, 0x2e, 0x31, 0x33, 0x61, 0x2e, 0x30, 0x37, 0x2e, 0x30, 0x37, 0x20, 0x30, 0x20, 0x30, 0x20, 0x30, 0x20, 0x30, 0x20, 0x2e, 0x30, 0x39, 0x20, 0x31, 0x37, 0x2e, 0x33, 0x32, 0x20, 0x31, 0x37, 0x2e, 0x33, 0x32, 0x20, 0x30, 0x20, 0x30, 0x20, 0x30, 0x20, 0x2e, 0x38, 0x33, 0x20, 0x31, 0x2e, 0x35, 0x37, 0x2e, 0x30, 0x37, 0x2e, 0x30, 0x37, 0x20, 0x30, 0x20, 0x30, 0x20, 0x30, 0x20, 0x2e, 0x30, 0x38, 0x20, 0x30, 0x20, 0x31, 0x36, 0x2e, 0x33, 0x39, 0x20, 0x31, 0x36, 0x2e, 0x33, 0x39, 0x20, 0x30, 0x20, 0x30, 0x20, 0x31, 0x20, 0x31, 0x2e, 0x38, 0x31, 0x2d, 0x2e, 0x35, 0x34, 0x20, 0x31, 0x35, 0x2e, 0x36, 0x35, 0x20, 0x31, 0x35, 0x2e, 0x36, 0x35, 0x20, 0x30, 0x20, 0x30, 0x20, 0x31, 0x20, 0x31, 0x31, 0x2e, 0x35, 0x39, 0x20, 0x31, 0x2e, 0x38, 0x38, 0x20, 0x31, 0x37, 0x2e, 0x35, 0x32, 0x20, 0x31, 0x37, 0x2e, 0x35, 0x32, 0x20, 0x30, 0x20, 0x30, 0x20, 0x30, 0x2d, 0x33, 0x2e, 0x37, 0x38, 0x20, 0x34, 0x2e, 0x34, 0x38, 0x63, 0x2d, 0x2e, 0x32, 0x2e, 0x33, 0x32, 0x2d, 0x2e, 0x33, 0x37, 0x2e, 0x36, 0x35, 0x2d, 0x2e, 0x35, 0x35, 0x20, 0x31, 0x73, 0x2d, 0x2e, 0x32, 0x32, 0x2e, 0x34, 0x35, 0x2d, 0x2e, 0x33, 0x33, 0x2e, 0x36, 0x39, 0x2d, 0x2e, 0x33, 0x31, 0x2e, 0x37, 0x32, 0x2d, 0x2e, 0x34, 0x34, 0x20, 0x31, 0x2e, 0x30, 0x38, 0x61, 0x31, 0x37, 0x2e, 0x34, 0x36, 0x20, 0x31, 0x37, 0x2e, 0x34, 0x36, 0x20, 0x30, 0x20, 0x30, 0x20, 0x30, 0x20, 0x34, 0x2e, 0x32, 0x39, 0x20, 0x31, 0x38, 0x2e, 0x37, 0x63, 0x2e, 0x32, 0x36, 0x2e, 0x32, 0x35, 0x2e, 0x35, 0x33, 0x2e, 0x34, 0x39, 0x2e, 0x38, 0x31, 0x2e, 0x37, 0x33, 0x73, 0x2e, 0x34, 0x34, 0x2e, 0x33, 0x37, 0x2e, 0x36, 0x37, 0x2e, 0x35, 0x34, 0x2e, 0x35, 0x39, 0x2e, 0x34, 0x34, 0x2e, 0x38, 0x39, 0x2e, 0x36, 0x34, 0x61, 0x2e, 0x30, 0x37, 0x2e, 0x30, 0x37, 0x20, 0x30, 0x20, 0x30, 0x20, 0x30, 0x20, 0x2e, 0x30, 0x38, 0x20, 0x30, 0x63, 0x2e, 0x33, 0x2d, 0x2e, 0x32, 0x31, 0x2e, 0x36, 0x2d, 0x2e, 0x34, 0x32, 0x2e, 0x38, 0x39, 0x2d, 0x2e, 0x36, 0x34, 0x73, 0x2e, 0x34, 0x35, 0x2d, 0x2e, 0x33, 0x35, 0x2e, 0x36, 0x37, 0x2d, 0x2e, 0x35, 0x34, 0x2e, 0x35, 0x35, 0x2d, 0x2e, 0x34, 0x38, 0x2e, 0x38, 0x31, 0x2d, 0x2e, 0x37, 0x33, 0x61, 0x31, 0x37, 0x2e, 0x34, 0x35, 0x20, 0x31, 0x37, 0x2e, 0x34, 0x35, 0x20, 0x30, 0x20, 0x30, 0x20, 0x30, 0x20, 0x35, 0x2e, 0x33, 0x38, 0x2d, 0x31, 0x32, 0x2e, 0x36, 0x31, 0x20, 0x31, 0x37, 0x2e, 0x33, 0x39, 0x20, 0x31, 0x37, 0x2e, 0x33, 0x39, 0x20, 0x30, 0x20, 0x30, 0x20, 0x30, 0x2d, 0x31, 0x2e, 0x30, 0x39, 0x2d, 0x36, 0x2e, 0x30, 0x39, 0x63, 0x2d, 0x2e, 0x31, 0x34, 0x2d, 0x2e, 0x33, 0x37, 0x2d, 0x2e, 0x32, 0x39, 0x2d, 0x2e, 0x37, 0x33, 0x2d, 0x2e, 0x34, 0x35, 0x2d, 0x31, 0x2e, 0x30, 0x39, 0x73, 0x2d, 0x2e, 0x32, 0x32, 0x2d, 0x2e, 0x34, 0x37, 0x2d, 0x2e, 0x33, 0x33, 0x2d, 0x2e, 0x36, 0x39, 0x2d, 0x2e, 0x33, 0x35, 0x2d, 0x2e, 0x36, 0x36, 0x2d, 0x2e, 0x35, 0x35, 0x2d, 0x31, 0x61, 0x31, 0x37, 0x2e, 0x36, 0x31, 0x20, 0x31, 0x37, 0x2e, 0x36, 0x31, 0x20, 0x30, 0x20, 0x30, 0x20, 0x30, 0x2d, 0x33, 0x2e, 0x37, 0x38, 0x2d, 0x34, 0x2e, 0x34, 0x38, 0x20, 0x31, 0x35, 0x2e, 0x36, 0x35, 0x20, 0x31, 0x35, 0x2e, 0x36, 0x35, 0x20, 0x30, 0x20, 0x30, 0x20, 0x31, 0x20, 0x31, 0x31, 0x2e, 0x36, 0x2d, 0x31, 0x2e, 0x38, 0x34, 0x20, 0x31, 0x36, 0x2e, 0x31, 0x33, 0x20, 0x31, 0x36, 0x2e, 0x31, 0x33, 0x20, 0x30, 0x20, 0x30, 0x20, 0x31, 0x20, 0x31, 0x2e, 0x38, 0x31, 0x2e, 0x35, 0x34, 0x2e, 0x30, 0x37, 0x2e, 0x30, 0x37, 0x20, 0x30, 0x20, 0x30, 0x20, 0x30, 0x20, 0x2e, 0x30, 0x38, 0x20, 0x30, 0x71, 0x2e, 0x34, 0x34, 0x2d, 0x2e, 0x37, 0x36, 0x2e, 0x38, 0x32, 0x2d, 0x31, 0x2e, 0x35, 0x36, 0x61, 0x2e, 0x30, 0x37, 0x2e, 0x30, 0x37, 0x20, 0x30, 0x20, 0x30, 0x20, 0x30, 0x20, 0x30, 0x2d, 0x2e, 0x30, 0x39, 0x41, 0x31, 0x36, 0x2e, 0x38, 0x39, 0x20, 0x31, 0x36, 0x2e, 0x38, 0x39, 0x20, 0x30, 0x20, 0x30, 0x20, 0x30, 0x20, 0x33, 0x30, 0x20, 0x2e, 0x35, 0x37, 0x7a, 0x22, 0x20, 0x66, 0x69, 0x6c, 0x6c, 0x3d, 0x22, 0x23, 0x31, 0x35, 0x31, 0x66, 0x33, 0x34, 0x22, 0x2f, 0x3e, 0x3c, 0x70, 0x61, 0x74, 0x68, 0x20, 0x64, 0x3d, 0x22, 0x4d, 0x32, 0x31, 0x2e, 0x38, 0x32, 0x20, 0x31, 0x37, 0x2e, 0x34, 0x37, 0x61, 0x31, 0x35, 0x2e, 0x35, 0x31, 0x20, 0x31, 0x35, 0x2e, 0x35, 0x31, 0x20, 0x30, 0x20, 0x30, 0x20, 0x31, 0x2d, 0x34, 0x2e, 0x32, 0x35, 0x20, 0x31, 0x30, 0x2e, 0x36, 0x39, 0x20, 0x31, 0x35, 0x2e, 0x36, 0x36, 0x20, 0x31, 0x35, 0x2e, 0x36, 0x36, 0x20, 0x30, 0x20, 0x30, 0x20, 0x31, 0x2d, 0x2e, 0x37, 0x32, 0x2d, 0x34, 0x2e, 0x36, 0x38, 0x20, 0x31, 0x35, 0x2e, 0x35, 0x20, 0x31, 0x35, 0x2e, 0x35, 0x20, 0x30, 0x20, 0x30, 0x20, 0x31, 0x20, 0x34, 0x2e, 0x32, 0x35, 0x2d, 0x31, 0x30, 0x2e, 0x36, 0x39, 0x20, 0x31, 0x35, 0x2e, 0x36, 0x32, 0x20, 0x31, 0x35, 0x2e, 0x36, 0x32, 0x20, 0x30, 0x20, 0x30, 0x20, 0x31, 0x20, 0x2e, 0x37, 0x32, 0x20, 0x34, 0x2e, 0x36, 0x38, 0x22, 0x20, 0x66, 0x69, 0x6c, 0x6c, 0x3d, 0x22, 0x23, 0x33, 0x34, 0x38, 0x35, 0x34, 0x30, 0x22, 0x2f, 0x3e, 0x3c, 0x70, 0x61, 0x74, 0x68, 0x20, 0x64, 0x3d, 0x22, 0x4d, 0x31, 0x35, 0x20, 0x32, 0x33, 0x2e, 0x34, 0x38, 0x61, 0x31, 0x35, 0x2e, 0x35, 0x35, 0x20, 0x31, 0x35, 0x2e, 0x35, 0x35, 0x20, 0x30, 0x20, 0x30, 0x20, 0x31, 0x2d, 0x2e, 0x37, 0x32, 0x20, 0x34, 0x2e, 0x36, 0x38, 0x20, 0x31, 0x35, 0x2e, 0x35, 0x34, 0x20, 0x31, 0x35, 0x2e, 0x35, 0x34, 0x20, 0x30, 0x20, 0x30, 0x20, 0x31, 0x2d, 0x33, 0x2e, 0x35, 0x33, 0x2d, 0x31, 0x35, 0x2e, 0x33, 0x37, 0x41, 0x31, 0x35, 0x2e, 0x35, 0x20, 0x31, 0x35, 0x2e, 0x35, 0x20, 0x30, 0x20, 0x30, 0x20, 0x31, 0x20, 0x31, 0x35, 0x20, 0x32, 0x33, 0x2e, 0x34, 0x38, 0x22, 0x20, 0x66, 0x69, 0x6c, 0x6c, 0x3d, 0x22, 0x23, 0x37, 0x64, 0x62, 0x63, 0x34, 0x32, 0x22, 0x2f, 0x3e, 0x3c, 0x2f, 0x73, 0x76, 0x67, 0x3e}, MediaType: "image/svg+xml"}, Description: ""},
Copy link
Member

Choose a reason for hiding this comment

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

Is the icon data important here?

Copy link
Contributor Author

@grokspawn grokspawn Oct 27, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Important in what sense? Here it's provided because the base data emulates the full range of what we would find in an official catalog.
It looks like it is not a mandatory element of either declcfg.Package or Model.

Copy link
Member

Choose a reason for hiding this comment

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

The point of this test is to test loading deprecations. Perhaps remove unnecessary test fixures that bloat the review and are not precise to what is being loaded. If I commit something that breaks icon loading, I'd love to only see the icon-related tests fail.

@grokspawn grokspawn force-pushed the deprecation_play branch 4 times, most recently from df88dab to bb139d8 Compare October 30, 2023 20:00
@grokspawn grokspawn marked this pull request as ready for review October 30, 2023 20:00
@openshift-ci 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 Oct 30, 2023
@openshift-ci openshift-ci bot requested review from kevinrizza and oceanc80 October 30, 2023 20:00
@grokspawn grokspawn changed the title poc for deprecation schema import deprecation schema & import Oct 30, 2023
@grokspawn grokspawn changed the title deprecation schema & import deprecation schema & import/render support Oct 30, 2023
@grokspawn grokspawn mentioned this pull request Oct 31, 2023
Closed
3 tasks
@grokspawn grokspawn force-pushed the deprecation_play branch 2 times, most recently from e9afb84 to 054146e Compare November 2, 2023 18:27
@grokspawn
poc for schema import
fd2c236 
Signed-off-by: Jordan Keister <[email protected]>
everettraven
everettraven approved these changes Nov 3, 2023
View reviewed changes
Copy link
Contributor

@everettraven everettraven left a comment

Choose a reason for hiding this comment

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

Changes look good to me

/lgtm

@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented Nov 6, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: everettraven, grokspawn, ncdc

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@ncdc ncdc added the lgtm Indicates that a PR is ready to be merged. label Nov 6, 2023
@openshift-merge-bot openshift-merge-bot bot merged commit f2285a2 into operator-framework:master Nov 6, 2023
10 of 12 checks passed
@grokspawn grokspawn deleted the deprecation_play branch November 6, 2023 20:14
joelanford
joelanford reviewed Nov 8, 2023
View reviewed changes
alpha/declcfg/declcfg.go
type DeprecationEntry struct {
Schema string `json:"schema"`
Name string `json:"name,omitempty"`
Message json.RawMessage `json:"message"`
Copy link
Member

Choose a reason for hiding this comment

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

I know this is already merged, but:

Suggested change
Message json.RawMessage `json:"message"`
Message string `json:"message"`

joelanford
joelanford reviewed Nov 8, 2023
View reviewed changes
alpha/declcfg/declcfg.go
SchemaPackage = "olm.package"
SchemaChannel = "olm.channel"
SchemaBundle = "olm.bundle"
SchemaDeprecation = "olm.catalog.deprecation"
Copy link
Member

Choose a reason for hiding this comment

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

Did we ever debate what the name of this should be? The most intuitive name that comes to mind for me is is olm.deprecations, but I can't remember if we had "catalog" in there for a reason.

@github-actions github-actions bot mentioned this pull request Nov 17, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stevekuznetsov stevekuznetsov stevekuznetsov left review comments

@joelanford joelanford joelanford left review comments

@everettraven everettraven everettraven approved these changes

@ncdc ncdc ncdc approved these changes

@kevinrizza kevinrizza Awaiting requested review from kevinrizza

@oceanc80 oceanc80 Awaiting requested review from oceanc80

Assignees
No one assigned
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@grokspawn @ncdc @joelanford @stevekuznetsov @everettraven