feat(property): implement compound dep constraints as bundle properties
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
This PR stands as a proof of concept/draft pending an enhancement.
Codecov Report
Merging #814 (57c0aff) into master (384af6b) will decrease coverage by
0.16%. The diff coverage is22.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
@@ 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 dataPowered by Codecov. Last update 384af6b...475fd64. Read the comment docs.
/cc @dinhxuanvu @joelanford
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 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.
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 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?
[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
- ~~OWNERS~~ [dinhxuanvu]
Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment
@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.
closing as stale