fix: Unable to deprecate bundles that are head of a NOT default channel with opm deprecatetruncate #780
Conversation
Codecov Report
@@ Coverage Diff @@
## master #780 +/- ##
==========================================
- Coverage 50.90% 50.64% -0.27%
==========================================
Files 102 102
Lines 8753 8848 +95
==========================================
+ Hits 4456 4481 +25
- Misses 3458 3523 +65
- Partials 839 844 +5
Continue to review full report at Codecov.
|
camilamacedo86
changed the title
openshift-ci
bot
added
the
do-not-merge/work-in-progress
…el with opm deprecatetruncate Signed-off-by: Camila Macedo <cmacedo@redhat.com>
…ot the pkg default Signed-off-by: Camila Macedo <cmacedo@redhat.com> (test): check that allows deprecate when the channel is not the default head Signed-off-by: Camila Macedo <cmacedo@redhat.com>
camilamacedo86
changed the title
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
| SELECT package_name FROM package, channel | ||
| WHERE channel.package_name == package.name | ||
| AND package.default_channel == channel.name | ||
| AND channel.head_operatorbundle_name = (SELECT name FROM operatorbundle WHERE bundlepath=? LIMIT 1) ` |
There was a problem hiding this comment.
See that it was returning the package when the bundle is HEAD of channel but the channel is NOT the default channel of the package, e.g:
SELECT package_name FROM package
INNER JOIN channel ON channel.name = package.default_channel
WHERE channel.head_operatorbundle_name = (SELECT name FROM operatorbundle WHERE bundlepath='quay.io/openshift-community-operators/apicurio-registry@sha256:239525b1a4a7030fb0184bea77a1e56b9bb652bbbb78e3facdedf829ee2f7e9d' LIMIT 1)
The select with inner join was wrong.
There was a problem hiding this comment.
Same thing here, the inner join was missing a compare on channel.package_name = package.name, this looks good.
| isDefaultChannelHead := `SELECT head_operatorbundle_name FROM package, channel | ||
| WHERE channel.package_name == package.name | ||
| AND package.default_channel == channel.name | ||
| AND channel.head_operatorbundle_name = ?` |
There was a problem hiding this comment.
see that the select with the inner join is wrong and is returning the head_operatorbundle_name when the bundle is head of a channel that is NOT the default channel of the package:
There was a problem hiding this comment.
I verified this, the inner join was missing a compare on package name.
SELECT head_operatorbundle_name FROM channel INNER JOIN package ON channel.name = package.default_channel AND channel.package_name = package.name WHERE channel.head_operatorbundle_name = ?
ankitathomas
left a comment
ankitathomas
left a comment
There was a problem hiding this comment.
Thanks for catching this, the change looks good!
| isDefaultChannelHead := `SELECT head_operatorbundle_name FROM package, channel | ||
| WHERE channel.package_name == package.name | ||
| AND package.default_channel == channel.name | ||
| AND channel.head_operatorbundle_name = ?` |
There was a problem hiding this comment.
I verified this, the inner join was missing a compare on package name.
SELECT head_operatorbundle_name FROM channel INNER JOIN package ON channel.name = package.default_channel AND channel.package_name = package.name WHERE channel.head_operatorbundle_name = ?
| SELECT package_name FROM package, channel | ||
| WHERE channel.package_name == package.name | ||
| AND package.default_channel == channel.name | ||
| AND channel.head_operatorbundle_name = (SELECT name FROM operatorbundle WHERE bundlepath=? LIMIT 1) ` |
There was a problem hiding this comment.
Same thing here, the inner join was missing a compare on channel.package_name = package.name, this looks good.
|
/lgtm |
| isDefaultChannelHead := `SELECT head_operatorbundle_name FROM channel | ||
| INNER JOIN package ON channel.name = package.default_channel | ||
| WHERE channel.head_operatorbundle_name = ?` | ||
| isDefaultChannelHead := `SELECT head_operatorbundle_name FROM package, channel |
There was a problem hiding this comment.
I took this issue as an opportunity to dust off my SQL sk1llz and here's what I've found:
The issue in the original query seems to be that the inner join is only made against the channel name and there are two channels (2.x and alpha). So, it's also joining the alpha rows with packages that have a default channel of alpha. An alternative query here would be to join on both the package name and the default channel. E.g.
SELECT head_operatorbundle_name FROM channel
INNER JOIN package ON channel.package_name = package.name AND channel.name = package.default_channel
WHERE channel.head_operatorbundle_name = ?
I believe the above and what you propose are equivalent in performance and intent. However, this StackOverflow entry seems to suggest that the inner join syntax is recommended. Not that this means you should change anything. Just relating what I've found ^^
There was a problem hiding this comment.
Oh, I didn't see the previous discussion on this. Feel free to ignore the comments above ^^
| @@ -0,0 +1,9 @@ | |||
| apiVersion: v1 | |||
There was a problem hiding this comment.
Just a question to help my understanding: why was it necessary to add all of these manifest and bundle files? why were they not there in the first place?
There was a problem hiding this comment.
I added the scenario for we are able to add a new test to ensure that after the change we not only break anything as it is working as supposed to be. You can see that are the same bundles from the community image.
There was a problem hiding this comment.
Most of these files don't directly impact the test at all.
@camilamacedo86 could you minimize the set of files added in this PR. This should only require CSVs and annotation files, right?
| @@ -0,0 +1,9 @@ | |||
| apiVersion: v1 | |||
There was a problem hiding this comment.
Most of these files don't directly impact the test at all.
@camilamacedo86 could you minimize the set of files added in this PR. This should only require CSVs and annotation files, right?
|
This change lgtm minus Nick's comments around testing |
|
HI @njhale and @kevinrizza, That the manifests were removed from the mocks. Feel free to check. |
Signed-off-by: Camila Macedo <cmacedo@redhat.com>
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ankitathomas, camilamacedo86, kevinrizza 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
bot
added
the
approved
|
/lgtm |
…el with opm deprecatetruncate (operator-framework#780) * fix: Unable to deprecate bundles that are head of a NOT default channel with opm deprecatetruncate Signed-off-by: Camila Macedo <cmacedo@redhat.com> * add test to ensure that the bundles are deprecated when the head is not the pkg default Signed-off-by: Camila Macedo <cmacedo@redhat.com> (test): check that allows deprecate when the channel is not the default head Signed-off-by: Camila Macedo <cmacedo@redhat.com> * removing uncessary manifests from the mock testdata Signed-off-by: Camila Macedo <cmacedo@redhat.com> Signed-off-by: akihikokuroda <akihikokuroda2020@gmail.com>
6 participants
Description
Fix Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=2001940
What was the problem
The wrong syntax with inner join returns the values for ANY head of channel and not only when it is the head of the channel which is the default of the package.
Problem faced (Example scenario)
Unable to deprecate the following bundles from the image registry.redhat.io/redhat/community-operator-index:v4.8.
apicurio-registry.v0.0.1 - defaultChannel:NO
apicurio-registry.v0.0.3-v1.2.3.final - defaultChannel:NO
apicurio-registry.v0.0.4-v1.3.2.final - defaultChannel:NO
When should deprecate since the image has the bundle: apicurio-registry-operator.v1.0.0-v2.0.0.final which is the head of the default channel 2.x which was not deprecated.
Testing the changes made in this PR
Regards usage of the method changed in this pr and possible impact which would require regression tests: