Skip to content

render: include olm.bundle.object properties in rendered bundle images #807

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

Conversation

joelanford
Copy link
Member

Signed-off-by: Joe Lanford [email protected]

Description of the change:
In opm render, include olm.bundle.object properties in rendered bundle images.

Motivation for the change:
By default, we need bundles to include olm.bundle.object properties so that the GRPC server can serve
them to OLM's packageserver component, which is used by OLM clients (e.g. the kubectl-operator plugin) to provide user-friendly installation workflows and display information about the operators available in catalogs installed in the cluster.

Note: packageserver only actually uses the CSVs from the heads of each channel. Catalog maintainers may opt to delete objects from non-channel heads to reduce the size of the catalog.

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 5, 2021
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Oct 5, 2021

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
Copy link
Contributor

openshift-ci bot commented Oct 5, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: joelanford

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

@openshift-ci openshift-ci bot requested review from awgreene and ecordell October 5, 2021 01:54
@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 5, 2021
@joelanford joelanford force-pushed the render-bundle-object-props branch from 3c066e6 to 32a1f1e Compare October 5, 2021 02:44
@jianzhangbjz
Copy link
Contributor

Thanks for your quick action!

Catalog maintainers may opt to delete objects from non-channel heads to reduce the size of the catalog.

Yes, this is a potential issue, but why not add a command or integrate some code to address this issue at this time? Or am I missing something? Thanks!

@joelanford joelanford force-pushed the render-bundle-object-props branch from 32a1f1e to 6bcfa75 Compare October 12, 2021 02:13
@joelanford joelanford force-pushed the render-bundle-object-props branch from 6bcfa75 to e7e9c01 Compare October 29, 2021 14:21
@codecov
Copy link

codecov bot commented Oct 29, 2021

Codecov Report

Merging #807 (c72969f) into master (d5522b3) will decrease coverage by 0.15%.
The diff coverage is 68.96%.

❗ Current head c72969f differs from pull request most recent head 5b19561. Consider uploading reports for the commit 5b19561 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master     #807      +/-   ##
==========================================
- Coverage   51.08%   50.92%   -0.16%     
==========================================
  Files         103      103              
  Lines        9056     8995      -61     
==========================================
- Hits         4626     4581      -45     
+ Misses       3553     3551       -2     
+ Partials      877      863      -14     
Impacted Files Coverage Δ
pkg/registry/registry_to_model.go 59.72% <65.38%> (+0.02%) ⬆️
alpha/action/render.go 64.06% <100.00%> (+0.18%) ⬆️
pkg/registry/csv.go 71.42% <0.00%> (-5.45%) ⬇️
pkg/registry/query.go 60.41% <0.00%> (-0.53%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d5522b3...5b19561. Read the comment docs.

@joelanford joelanford force-pushed the render-bundle-object-props branch from e7e9c01 to 1b07b54 Compare October 29, 2021 14:53
@joelanford joelanford marked this pull request as ready for review October 29, 2021 15:41
@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 29, 2021
@joelanford
Copy link
Member Author

Yes, this is a potential issue, but why not add a command or integrate some code to address this issue at this time? Or am I missing something? Thanks!

This is a completely reasonable thing to do, and you aren't missing anything.

We're methodically designing and building out the pieces that catalog maintainers will likely need, and we want to make sure we build the right abstractions. This "prune unnecessary bundle objects" feature likely falls in the broader category of catalog post-processing and optimizations, so we're holding off on integrating specific post-processors until we have a more holistic idea of how all the catalog pipeline pieces fit together.

@joelanford joelanford force-pushed the render-bundle-object-props branch from 1b07b54 to 5b19561 Compare October 29, 2021 16:25
@@ -296,22 +296,33 @@ func populateDBRelatedImages(ctx context.Context, cfg *declcfg.DeclarativeConfig
}

func bundleToDeclcfg(bundle *registry.Bundle) (*declcfg.DeclarativeConfig, error) {
bundleProperties, err := registry.PropertiesFromBundle(bundle)
objs, props, err := registry.ObjectsAndPropertiesFromBundle(bundle)
Copy link
Member Author

Choose a reason for hiding this comment

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

This is the breaking change that go-apidiff is complaining about. This function was introduced with FBC code, so it hasn't been in the wild long.

I'm perfectly willing to refactor to avoid a breaking change if others think its necessary here.

@kevinrizza
Copy link
Member

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Nov 1, 2021
@openshift-merge-robot openshift-merge-robot merged commit 7c8187a into operator-framework:master Nov 1, 2021
@joelanford joelanford deleted the render-bundle-object-props branch November 16, 2021 21:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Development

Successfully merging this pull request may close these issues.

4 participants