operator-registry icon indicating copy to clipboard operation
operator-registry copied to clipboard

feat(property): implement compound dep constraints as bundle properties

Open estroz opened this issue 4 years ago • 9 comments

Description of the change: add support for the olm.constraint bundle property in alpha diff.

Motivation for the change: this is the only change needed to handle the new constraint type in opm, aside from potential validation.

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

/kind feature

estroz avatar Oct 11 '21 19:10 estroz

This PR stands as a proof of concept/draft pending an enhancement.

estroz avatar Oct 11 '21 19:10 estroz

Codecov Report

Merging #814 (57c0aff) into master (384af6b) will decrease coverage by 0.16%. The diff coverage is 22.22%.

:exclamation: Current head 57c0aff differs from pull request most recent head 475fd64. Consider uploading reports for the commit 475fd64 to get more accurate results Impacted file tree graph

@@            Coverage Diff             @@
##           master     #814      +/-   ##
==========================================
- Coverage   52.07%   51.91%   -0.17%     
==========================================
  Files         103      103              
  Lines        9092     9096       +4     
==========================================
- Hits         4735     4722      -13     
- Misses       3449     3466      +17     
  Partials      908      908              
Impacted Files Coverage Δ
alpha/declcfg/diff.go 69.37% <22.22%> (-7.55%) :arrow_down:
pkg/registry/types.go 18.57% <0.00%> (-9.84%) :arrow_down:
pkg/lib/bundle/validate.go 60.69% <0.00%> (+0.52%) :arrow_up:

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 384af6b...475fd64. Read the comment docs.

codecov[bot] avatar Oct 11 '21 19:10 codecov[bot]

/cc @dinhxuanvu @joelanford

estroz avatar Oct 11 '21 19:10 estroz

It would be great if you could expand a little bit in the motivation section of the PR. For instance, which dependency types currently exist, how they are specified, why they aren't enough (i.e. what use-case(s) can be enabled with this new feature) and what they would look like.

(I might be misunderstanding something, so please be patient with me) The other thing that's confusing me a bit is that with package dependencies, we should already have AND and NOT, right? e.g.

dependencies:
  - type: olm.package
    value:
      packageName: packageA
      version: ">=1.0.0"
  - type: olm.package
    value:
      packageName: packageB
      version: "<2.0.0"

could be understood as packageA>=1.0.0 AND packageB <2.0.0 or even packageA>=1.0.0 AND NOT packageB>=2.0.0

perdasilva avatar Oct 18 '21 14:10 perdasilva

@perdasilva once an EP is published and merged I will add more details; the EP will discuss prior art and what the new compound spec is.

estroz avatar Oct 18 '21 18:10 estroz

Is there a reason for operator-registry to need to know about these constraints? In general (for FBC at least), operator-registry only explicitly parses dependencies and properties that it needs to translate or validate for its own purposes of building a valid index. Other properties are just blindly included in the index to be forwarded on to GRPC clients.

Do we think operator-registry will need to have explicit knowledge of every constraint type? Right now, the olm.maxOpenshiftVersion constraint is unknown to operator-registry AFAIK.

joelanford avatar Oct 21 '21 01:10 joelanford

@joelanford only for diff really, if properties are blindly forwarded. Could the stuff handling GVK/package dependencies in pkg/api and pkg/registry be mostly removed then?

estroz avatar Nov 10 '21 17:11 estroz

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 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[bot] avatar Dec 15 '21 22:12 openshift-ci[bot]

@estroz: PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

openshift-ci[bot] avatar Apr 13 '22 22:04 openshift-ci[bot]

closing as stale

perdasilva avatar Feb 19 '24 13:02 perdasilva