Bug 1889388: Set replaces in ListBundles query result using channel entries. #483
Conversation
openshift-ci-robot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: benluddy 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:
Approvers can indicate their approval by writing |
openshift-ci-robot
added
approved
|
@benluddy: This pull request references Bugzilla bug 1889388, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker. 3 validation(s) were run on this bug
In response to this:
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
removed
the
do-not-merge/work-in-progress
|
@benluddy: This pull request references Bugzilla bug 1889388, which is valid. 3 validation(s) were run on this bug
In response to this:
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. |
benluddy
force-pushed
the
listbundles-channel-entry-replacement
branch
from
dba076d
to
33f6275
Compare
|
/lgtm |
| LEFT OUTER JOIN dependencies ON dependencies.operatorbundle_name = channel_entry.operatorbundle_name | ||
| LEFT OUTER JOIN properties ON properties.operatorbundle_name = channel_entry.operatorbundle_name | ||
| INNER JOIN package ON package.name = channel_entry.package_name` | ||
| const listBundlesQuery = `SELECT DISTINCT channel_entry.entry_id, operatorbundle.bundle, operatorbundle.bundlepath, |
There was a problem hiding this comment.
My only question is does this change to the query impact non-semver bundles in any way? Is that query behavior the same as before. Or are we saying in the latest versions of opm we expect the bundles to be built in semver mode?
There was a problem hiding this comment.
For replaces bundles, the existing query should work the same either way, since the replaces column in channel_entry should be synthesized from the bundle. The existing query is only problematic for semver indices, since in that case the bundle and channel_entry "replaces" column aren't necessarily the same.
@kevinrizza Please check my understanding. I put a hold on this to wait for confirmation.
There was a problem hiding this comment.
You're right @benluddy, in both cases the channel_entry table is correct -- the source of truth should be the edges listed in the channel_entry table. The only diff is that in semver mode the replaces field is ignored when deciding what to add. In replaces mode that table gets rebuilt from scratch every time, so it should always be correct.
|
/hold |
openshift-ci-robot
added
the
do-not-merge/hold
benluddy
requested review from
kevinrizza
and removed request for
Bowenislandsong
|
/hold cancel |
openshift-ci-robot
removed
the
do-not-merge/hold
| LEFT OUTER JOIN properties ON properties.operatorbundle_name = channel_entry.operatorbundle_name | ||
| INNER JOIN package ON package.name = channel_entry.package_name` | ||
| const listBundlesQuery = `SELECT DISTINCT channel_entry.entry_id, operatorbundle.bundle, operatorbundle.bundlepath, | ||
| channel_entry.operatorbundle_name, channel_entry.package_name, channel_entry.channel_name, replaced_entry.operatorbundle_name, operatorbundle.skips, |
There was a problem hiding this comment.
This is definitely better, but in theory we might want to synthesize all of the channel edges directly from just the channel_entry table given that these input fields are disparate and may or may not be relevant when deciding what edges are actually real. That's the intention of the channel_entry table itself.
That being said, I think that's something we can worry about later and for now this is better.
|
/lgtm |
|
@benluddy: All pull requests linked via external trackers have merged: Bugzilla bug 1889388 has been moved to the MODIFIED state. In response to this:
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. |
|
/cherry-pick release-4.6 |
|
@benluddy: new pull request created: #497 In response to this:
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. |
The "replaces" column in ListBundles query result rows was being populated with the "replaces" from the respective bundle. For registries built using semver mode, the source of truth for "replaces" is the "replaces" column on each channel entry row.