search

operator-framework / operator-courier

Build, verify and push operators
Apache License 2.0
41 stars 53 forks source link

courier: fix validation with multiple CRD versions #194

Closed ffromani closed 3 years ago

ffromani commented 3 years ago

If we ship a bundle which have multiple CRD version, without this patch the validation may fail with a false negative with

CRD.spec.version does not match CSV.spec.crd.owned.version

This is because we check that EVERY owned CRD matches with the supported version in the CSV. But this doesn't work if we supply a CSV which points to a CRD version (example: v1) which is in the bundle, but which also contains older CRD versions.

IOW the failed scenario is:

CSV:
  owns: v1, v1alpha1
CRD:
  provides: v1, v1alpha1
  supports: v1

here "supports" mean the value listed in the version field of the CRD.

In this case the need to check:

  1. the CRD version (v1) is owned by the CSV
  2. all the versions owned in the CSV are declared in the CRD

The check #2 was already guarantee by the code looping over CSV owned CRD versions; this patch fixes the code to fix the check #1.

Related-To: https://bugzilla.redhat.com/show_bug.cgi?id=1869525 Signed-off-by: Francesco Romani fromani@redhat.com

openshift-ci-robot commented 3 years ago

Hi @fromanirh. Thanks for your PR.

I'm waiting for a operator-framework member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
ffromani commented 3 years ago

/hold adding tests

ffromani commented 3 years ago

/hold cancel really ready to review now

kevinrizza commented 3 years ago

/ok-to-test

kevinrizza commented 3 years ago

/approve /lgtm

MarSik commented 3 years ago

/retest

yanirq commented 3 years ago

/retest

ffromani commented 3 years ago

it seems to me the CI jobs are not even starting :\

kevinrizza commented 3 years ago

@fromanirh Seems like CI is just stuck, can you try closing and reopening the pr to see if that restarts the tests?

ffromani commented 3 years ago

@fromanirh Seems like CI is just stuck, can you try closing and reopening the pr to see if that restarts the tests?

sure thing: https://github.com/operator-framework/operator-courier/pull/195

ffromani commented 3 years ago

ah, nice, it seems just reopening this one did the trick. Closing #195 for now.

kevinrizza commented 3 years ago

@fromanirh seems like there is some kind of permissions problem with our CI (the prow job wasn't initializing and our travis ci integration test runner was not attaching to prs for some reason). It seems like the latest run is actually working, but in the meantime the job is actually failing: https://travis-ci.org/github/operator-framework/operator-courier/builds/719262364

Seems like it is just a small formatting issue that flake8 is seeing in python 3.7:

./operatorcourier/validate.py:349:21: E128 continuation line under-indented for visual indent

Could you update your pr to resolve that and then we can get it merged? the functional tests are all passing.

ffromani commented 3 years ago

@fromanirh seems like there is some kind of permissions problem with our CI (the prow job wasn't initializing and our travis ci integration test runner was not attaching to prs for some reason). It seems like the latest run is actually working, but in the meantime the job is actually failing: https://travis-ci.org/github/operator-framework/operator-courier/builds/719262364

Seems like it is just a small formatting issue that flake8 is seeing in python 3.7:

./operatorcourier/validate.py:349:21: E128 continuation line under-indented for visual indent

Could you update your pr to resolve that and then we can get it merged? the functional tests are all passing.

sure, fixing the flake8 issue. Thanks for letting me know, I somehow missed it so far.

openshift-ci-robot commented 3 years ago

New changes are detected. LGTM label has been removed.