Migration of the diff logic to oc-mirror

[pawicao-ibm]

This PR is a follow-up of #969 and is being worked on simultaneously with openshift/oc-mirror#497

Description of the change:
The discussion on the diff logic started with openshift/oc-mirror#453 issue where the defaultChannels for operators where invalid while using minVersion/maxVersion.

Further work and discussions on adding the Priority property (see #969) resulted in a conclusion to migrate the entirety of diff logic to oc-mirror.

This work has been prepared in the following PR: openshift/oc-mirror#497

Some changes in the operator-registry have to be done though together with the migration (e.g. changing schema variables to public or adding properties to a channel model), in order for oc-mirror's diff to be able to take advantage of needed operator-registry structures.

Lastly this migration results in changes to opm diff. See here for the latest thoughts on deprecating this command

Motivation for the change:

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

[openshift-ci]

Hi @pawicao-ibm. 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.

[dinhxuanvu]

/ok-to-test

[dinhxuanvu]

Please do not merge this PR until the oc-mirror PR is merged. I will put a hold here just in case.
/hold

[dinhxuanvu]

I would like to ask you @pawicao-ibm to squash the commits to reduce the noises here. Grouping the commits together based on their purposes to reduce maybe 2 main commits (one for adding channel properties and one for removing the diff logic).
I would prefer having the two proposed commits to be 2 separate PRs given they are not necessarily linked together but I'll let others chime in.
@joelanford FYI

[codecov]

Codecov Report

Merging #1006 (1096598) into master (33ed9ea) will decrease coverage by 1.19%.
The diff coverage is 85.71%.

@@            Coverage Diff             @@
##           master    #1006      +/-   ##
==========================================
- Coverage   52.87%   51.67%   -1.20%     
==========================================
  Files         104      101       -3     
  Lines        9468     8961     -507     
==========================================
- Hits         5006     4631     -375     
+ Misses       3529     3450      -79     
+ Partials      933      880      -53     

Impacted Files	Coverage Δ
alpha/declcfg/declcfg.go	100.00% <ø> (ø)
alpha/declcfg/load.go	78.65% <66.66%> (ø)
alpha/declcfg/model_to_declcfg.go	100.00% <100.00%> (ø)
pkg/registry/query.go	61.45% <0.00%> (-0.53%)	⬇️

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

[dinhxuanvu]

One a second look, in fact there is already a PR for priority channel which is linked here (#969) so I recommend to move all commits associated to that PR out of this PR and squash the rest into one.

[pawicao-ibm]

It made sense and I followed your suggestion. This PR now includes the migration of the diff logic to oc-mirror, #969 includes only adding the channel priority.

There are two things to note here:

1. Unfortunately it is not that easy to just wait for oc-mirror PR to be merged first as it also depends on changes we are making here - refactoring some variables to public in order for them to be accessible from other projects (schemas mostly)
2. The question here is what would be done with opm alpha diff. This PR deletes it completely. Should it stay here, have a dependency to diff from oc-mirror and be left with a deprecation notice for some time? Or is the complete removal okay?

[dinhxuanvu]

It made sense and I followed your suggestion. This PR now includes the migration of the diff logic to oc-mirror, #969 includes only adding the channel priority.

There are two things to note here:

1. Unfortunately it is not that easy to just wait for oc-mirror PR to be merged first as it also depends on changes we are making here - refactoring some variables to public in order for them to be accessible from other projects (schemas mostly)
2. The question here is what would be done with opm alpha diff. This PR deletes it completely. Should it stay here, have a dependency to diff from oc-mirror and be left with a deprecation notice for some time? Or is the complete removal okay?

If you remove the diff code, then alpha diff command on opm will need to get removed as well.

[pawicao-ibm]

If you remove the diff code, then alpha diff command on opm will need to get removed as well.

yes and that's exactly how it is prepared now. I was asking if for sure you are okay with that

[dinhxuanvu]

/hold cancel

[dinhxuanvu]

@pawicao-ibm Hey Oskar, do me a favor and do a few things for this PR:

• Rebase against master
• Run make vendor and ensure the sanity check pass
• Sign your commit

Thanks.

[pawicao-ibm]

@pawicao-ibm Hey Oskar, do me a favor and do a few things for this PR:

* Rebase against master

* Run `make vendor` and ensure the `sanity` check pass

* Sign your commit

Thanks.

Is it okay now?

[dinhxuanvu]

/approve

[dinhxuanvu]

@joelanford Can you please take a look and give it a lgtm if everything is good? Thanks.

[grokspawn]

Appreciate it if we can run to ground the new exports before concluding this. Unless required by an API consumer not represented here, we'd like to minimize exports when possible.

[dinhxuanvu]

Appreciate it if we can run to ground the new exports before concluding this. Unless required by an API consumer not represented here, we'd like to minimize exports when possible.

I'm pretty sure the diff code uses them. Now the diff code is moving out of the package, these variables and functions need to be exportable so they can be used on mirror repo.

[dinhxuanvu]

@pawicao-ibm Can you confirm that you need those variables and the func to be exportable for mirror usage? Or there is another reason for it?

[pawicao-ibm]

Now that I look into that, they are used but within tests of the diff logic, while creating DeclarativeConfigs. They are also used here: https://github.com/openshift/oc-mirror/blob/a1440e623b1f6c9652c3053ec78661bfe223ecae/pkg/operator/diff/internal/diff.go#L542 - this function is called for comparing bundles. Maybe there would be a way around that tho

[dinhxuanvu]

Now that I look into that, they are used but within tests of the diff logic, while creating DeclarativeConfigs. They are also used here: https://github.com/openshift/oc-mirror/blob/a1440e623b1f6c9652c3053ec78661bfe223ecae/pkg/operator/diff/internal/diff.go#L542 - this function is called for comparing bundles. Maybe there would be a way around that tho

I see. The main question is if these changes are needed. If they ain't, we should reverse them. I personally don't mind the changes but let keep the changes to the minimum and avoid unnecessary ones.

[pawicao-ibm]

I am looking into that

[pawicao-ibm]

Now that I look into that, they are used but within tests of the diff logic, while creating DeclarativeConfigs. They are also used here: https://github.com/openshift/oc-mirror/blob/a1440e623b1f6c9652c3053ec78661bfe223ecae/pkg/operator/diff/internal/diff.go#L542 - this function is called for comparing bundles. Maybe there would be a way around that tho

I see. The main question is if these changes are needed. If they ain't, we should reverse them. I personally don't mind the changes but let keep the changes to the minimum and avoid unnecessary ones.

These changes are needed if we want to have the diff functions in oc-mirror and not have redundant duplications of these schemas. I see some possibility of manouvering functions here and there. For example some of the internal diff functions (https://github.com/openshift/oc-mirror/blob/a1440e623b1f6c9652c3053ec78661bfe223ecae/pkg/operator/diff/internal/diff.go#L542:L629) could go to operator-registry - model.go or model_to_declcfg.go yet in the end they would have to be exported anyway

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dinhxuanvu, grokspawn, pawicao-ibm

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:

• OWNERS [dinhxuanvu,grokspawn]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[grokspawn]

Appreciate the diligence, folks.

/lgtm