fix(render/diff): Support olm.constraint in opm render/diff cmd

[dinhxuanvu]

Currently, the olm.constraint type is ignored and not recognized in
render and diff cmd. This commit is to add supoprt for constraint
type.

Signed-off-by: Vu Dinh vudinh@outlook.com

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]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dinhxuanvu

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:

• OWNERS [dinhxuanvu]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[codecov]

Codecov Report

Merging #901 (e5f9135) into master (611525b) will increase coverage by 0.11%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #901      +/-   ##
==========================================
+ Coverage   52.07%   52.18%   +0.11%     
==========================================
  Files         103      103              
  Lines        9092     9113      +21     
==========================================
+ Hits         4735     4756      +21     
- Misses       3449     3451       +2     
+ Partials      908      906       -2     

Impacted Files	Coverage Δ
pkg/registry/types.go	28.40% <ø> (ø)
pkg/registry/registry_to_model.go	62.33% <100.00%> (+2.61%)	⬆️
pkg/lib/registry/registry.go	18.39% <0.00%> (-0.36%)	⬇️
pkg/lib/indexer/indexer.go	10.61% <0.00%> (-0.18%)	⬇️
alpha/declcfg/diff.go	77.96% <0.00%> (+1.04%)	⬆️
pkg/lib/bundle/exporter.go	62.85% <0.00%> (+6.19%)	⬆️
alpha/declcfg/diff_include.go	67.92% <0.00%> (+6.60%)	⬆️

---

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 611525b...e5f9135. Read the comment docs.

[joelanford]

I might be missing something, but why do we need to parse/recognize the olm.constraint properties in the opm render/diff commands?

(I'm assuming that even without this change, those properties are treated opaquely and carried along without any processing)

[dinhxuanvu]

@joelanford Apparently, olm.constraint is ignored by render/diff command for some reason. I'm still testing it out with these changes.

[Xia-Zhao-rh]

Hi, @dinhxuanvu
For "opm render", maybe It is needed to add default p.Type here, https://github.com/operator-framework/operator-registry/blob/master/pkg/registry/registry_to_model.go#L57

var packageRequiredProps []property.Property
for i, p := range b.Dependencies {
	switch p.Type {
	case property.TypeGVK:
		...
	case property.TypePackage:
		...
	default:
		packageRequiredProps = append(packageRequiredProps, property.Property{
			Type:  p.Type,
			Value: p.Value,
		})
	}

[dinhxuanvu]

@joelanford Ready for the final review.

[joelanford]

Just to make sure I'm understanding this, the reason we need this logic in the conversion code is because olm.constraints are encoded in bundles as dependencies instead of properties?

I was under the impression that olm.constraint was always meant to be introduced as a property, but I also wasn't paying that much attention to that aspect of it.

[dinhxuanvu]

Just to make sure I'm understanding this, the reason we need this logic in the conversion code is because olm.constraints are encoded in bundles as dependencies instead of properties?

I was under the impression that olm.constraint was always meant to be introduced as a property, but I also wasn't paying that much attention to that aspect of it.

The EP said that olm.constraint is specified in dependencies.yaml as it is intended to be a generic constraint/dependency. If they can be included in properties.yaml, it will still work. The fact we do some consolidation behind the scene between dependencies and properties was merely a grpc implementation detail which frankly we just did as a bug fix, not as a proper enhancement change. Up to this point, we still use olm.gvk and olm.package (and now olm.constraint) dependencies in dependencies.yaml and other arbitrary properties are in properties.yaml. I prefer to keep the convention as it is until we have a plan to consolidate it officially and perhaps deprecate dependencies.yaml altogether.

[joelanford]

/lgtm