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.

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

fix(opm): clarify that bundle declcfgs are not valid refs alone #741

Merged
merged 2 commits into from
Aug 11, 2021

Conversation

estroz
Copy link
Member

@estroz estroz commented Aug 5, 2021

Description of the change: clarify that bundle declcfgs are not valid refs alone, and require an olm.package declcfg. An error is returned if a bundle is directly specified.

Motivation for the change: diff is really intended for indices. There's no way to infer which channel is the default for a package if more than one bundle in the same package is passed to diff either, so we could get non-deterministic behavior unless diff imposes some ordering on channels. Assuming icon doesn't exist (or other future olm.package fields) for a package may cause issues when using the diff'ed catalog in-cluster too.

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

@codecov
Copy link

codecov bot commented Aug 5, 2021

Codecov Report

Merging #741 (8100604) into master (d446d3c) will decrease coverage by 0.13%.
The diff coverage is 61.53%.

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

@@            Coverage Diff             @@
##           master     #741      +/-   ##
==========================================
- Coverage   50.30%   50.16%   -0.14%     
==========================================
  Files         100      100              
  Lines        8578     8535      -43     
==========================================
- Hits         4315     4282      -33     
+ Misses       3430     3425       -5     
+ Partials      833      828       -5     
Impacted Files Coverage Δ
internal/action/list.go 67.70% <0.00%> (ø)
internal/action/diff.go 41.93% <42.85%> (-4.22%) ⬇️
internal/action/render.go 60.94% <100.00%> (-0.23%) ⬇️
pkg/lib/registry/registry.go 13.33% <0.00%> (-11.05%) ⬇️

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 d446d3c...f20d176. Read the comment docs.

Copy link
Contributor

@ankitathomas ankitathomas left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Aug 5, 2021
oldCfg, err := oldRender.Run(ctx)
if err != nil {
if isNotAllowedError(err) {
Copy link
Member

Choose a reason for hiding this comment

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

Can you add unit tests to cover both old and new refs being bundles?

@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Aug 5, 2021
@joelanford
Copy link
Member

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Aug 6, 2021
Copy link
Member

@awgreene awgreene left a comment

Choose a reason for hiding this comment

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

Nice work @estroz, a few nits.

cmd/opm/alpha/diff/cmd.go Outdated Show resolved Hide resolved
func (_ *ErrNotAllowed) Error() string {
return "not allowed"
}
var ErrNotAllowed = errors.New("not allowed")
Copy link
Member

Choose a reason for hiding this comment

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

+1

internal/action/render.go Outdated Show resolved Hide resolved
internal/action/render.go Outdated Show resolved Hide resolved
Signed-off-by: Eric Stroczynski <ericstroczynski@gmail.com>
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Aug 10, 2021
….Is() comparison

Signed-off-by: Eric Stroczynski <ericstroczynski@gmail.com>
Copy link
Member

@dinhxuanvu dinhxuanvu left a comment

Choose a reason for hiding this comment

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

/approve

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Aug 11, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ankitathomas, dinhxuanvu, estroz

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 added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 11, 2021
@joelanford
Copy link
Member

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Aug 11, 2021
@openshift-ci openshift-ci bot merged commit 88d4d76 into operator-framework:master Aug 11, 2021
@estroz estroz deleted the docs/clarify-diff-ref branch August 11, 2021 19:41
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.

5 participants