Skip to content

Conversation

@jchunkins
Copy link
Contributor

@jchunkins jchunkins commented May 1, 2021

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 jhunkins@us.ibm.com

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 #640

- 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

Implements operator-framework#640

Signed-off-by: John Hunkins <jhunkins@us.ibm.com>
@openshift-ci-robot
Copy link

[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
Copy link

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 openshift-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label May 1, 2021
@codecov
Copy link

codecov bot commented May 1, 2021

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%) ⬆️
pkg/lib/validation/bundle.go 100.00% <100.00%> (+50.90%) ⬆️

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

@dinhxuanvu
Copy link
Member

/ok-to-test

@openshift-ci-robot openshift-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels May 3, 2021
@jchunkins
Copy link
Contributor Author

/test all

@openshift-ci-robot
Copy link

@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.

@jchunkins
Copy link
Contributor Author

/test ?

@openshift-ci-robot
Copy link

@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.

@timflannagan
Copy link
Member

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.

@jchunkins
Copy link
Contributor Author

Thanks @timflannagan

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jun 3, 2022

@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 openshift-ci bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 3, 2022
@awgreene
Copy link
Member

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 awgreene closed this Aug 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. ok-to-test Indicates a non-member PR verified by an org member that is safe to test.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bundle validation discrepancies between operator-registry and api projects

5 participants