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

Remove redundant validateOwnedCRDs impl

Open jchunkins opened this issue 3 years ago • 11 comments

Description of the change:

  • Adjust RegistryBundleValidator to remove the duplicate implementation for validating Owned CRDs that operates on the operator registry bundle type.
  • Leave RegistryBundleValidator skeleton in place for future registry bundle validation
  • Adjust existing unit test to account for validation removal
  • Make ValidateBundleContent use the registry bundle AND api bundle validators
  • Fix bug in argument handling for ObjectValidator which would cause validation to never trigger because it was passing an array itself instead of expanding ... to pass to Variadic argument
  • enhance test coverage for files that have changed

Signed-off-by: John Hunkins [email protected]

Motivation for the change:

Remove redundant "validate owned CRD" implementation from operator registry while keeping a future point of expansion for registry specific validation. This consolidates the "validate owned CRD" into a single implementation which already exists in the api project.

Also used this as an opportunity to enhance code coverage.

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

Closes https://github.com/operator-framework/operator-registry/issues/640

jchunkins avatar May 01 '21 01:05 jchunkins

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: jchunkins To complete the pull request process, please assign ecordell after the PR has been reviewed. You can assign the PR to them by writing /assign @ecordell in a comment when ready.

The full list of commands accepted by this bot can be found 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-robot avatar May 01 '21 01:05 openshift-ci-robot

Hi @jchunkins. 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. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

openshift-ci-robot avatar May 01 '21 01:05 openshift-ci-robot

Codecov Report

Merging #650 (7ffa364) into master (cbcacd3) will increase coverage by 0.51%. The diff coverage is 86.66%.

@@            Coverage Diff             @@
##           master     #650      +/-   ##
==========================================
+ Coverage   48.40%   48.91%   +0.51%     
==========================================
  Files          87       87              
  Lines        7599     7562      -37     
==========================================
+ Hits         3678     3699      +21     
+ Misses       3195     3155      -40     
+ Partials      726      708      -18     
Impacted Files Coverage Δ
pkg/lib/bundle/validate.go 74.57% <85.71%> (+14.22%) :arrow_up:
pkg/lib/validation/bundle.go 100.00% <100.00%> (+50.90%) :arrow_up:

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

codecov[bot] avatar May 01 '21 01:05 codecov[bot]

/ok-to-test

dinhxuanvu avatar May 03 '21 20:05 dinhxuanvu

/test all

jchunkins avatar May 03 '21 21:05 jchunkins

@jchunkins: No presubmit jobs available for operator-framework/operator-registry@master

In response to this:

/test all

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-robot avatar May 03 '21 21:05 openshift-ci-robot

/test ?

jchunkins avatar May 03 '21 21:05 jchunkins

@jchunkins: No presubmit jobs available for operator-framework/operator-registry@master

In response to this:

/test ?

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-robot avatar May 03 '21 21:05 openshift-ci-robot

I took a quick look at the changes and they seem reasonable but my brain is dead for the day. Going to try and carve out sometime later this week to take another look.

timflannagan avatar May 03 '21 21:05 timflannagan

Thanks @timflannagan

jchunkins avatar May 03 '21 21:05 jchunkins

@jchunkins: 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 Jun 03 '22 22:06 openshift-ci[bot]

This PR has been open for over a year and seems stale. I am going to close this PR for now, but if the suggested features are still desired, please reopen.

awgreene avatar Aug 30 '22 16:08 awgreene