Skip to content

Conversation

@ankitathomas
Copy link
Contributor

--overwrite-latest removes the head bundle being overwritten, setting channel head to either the previous bundle in replaces chain or dropping the channel entirely. However, when a single bundle default channel head is overwritten, this causes the package to lose its default channel, causing the package manifest to be unable to determine the default channel unless the overwritten bundle is both the highest semver in the package and either has one channel or has a default channel annotation.

This PR retains a stub default channel entry to help determine a default channel after the overwrite

@openshift-ci openshift-ci bot requested review from ecordell and kevinrizza October 8, 2021 19:18
Signed-off-by: Ankita Thomas <ankithom@redhat.com>
@ankitathomas ankitathomas force-pushed the overwrite-channel-drop-fix branch from f656b61 to 995885d Compare October 8, 2021 19:18
@codecov
Copy link

codecov bot commented Oct 8, 2021

Codecov Report

Merging #812 (48ca8a3) into master (4f790f4) will increase coverage by 0.00%.
The diff coverage is 75.00%.

❗ Current head 48ca8a3 differs from pull request most recent head 9bf6813. Consider uploading reports for the commit 9bf6813 to get more accurate results
Impacted file tree graph

@@           Coverage Diff           @@
##           master     #812   +/-   ##
=======================================
  Coverage   51.04%   51.04%           
=======================================
  Files         103      103           
  Lines        9049     9050    +1     
=======================================
+ Hits         4619     4620    +1     
  Misses       3553     3553           
  Partials      877      877           
Impacted Files Coverage Δ
pkg/sqlite/load.go 43.24% <0.00%> (ø)
alpha/declcfg/declcfg_to_model.go 92.13% <100.00%> (+0.08%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4f790f4...9bf6813. Read the comment docs.

Copy link
Member

@njhale njhale left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested this out locally against the 4.6 index and affected package with positive results:

$ opm index add --enable-alpha --generate --bundles registry-proxy.engineering.redhat.com/rh-osbs/distributed-tracing-jaeger-operator-bundle@sha256:bf951190f9c71d3bf7776145be28dff46782efefee65525da845d5474a8b8bab --from-index registry-proxy.engineering.redhat.com/rh-osbs/iib-pub-pending:v4.6 --overwrite-latest
...
INFO[0000] building the index                            bundles="[registry-proxy.engineering.redhat.com/rh-osbs/distributed-tracing-jaeger-operator-bundle@sha256:bf951190f9c71d3bf7776145be28dff46782efefee65525da845d5474a8b8bab]"
INFO[0000] Pulling previous image registry-proxy.engineering.redhat.com/rh-osbs/iib-pub-pending:v4.6 to get metadata  bundles="[registry-proxy.engineering.redhat.com/rh-osbs/distributed-tracing-jaeger-operator-bundle@sha256:bf951190f9c71d3bf7776145be28dff46782efefee65525da845d5474a8b8bab]"
...
INFO[0037] writing dockerfile: index.Dockerfile          bundles="[registry-proxy.engineering.redhat.com/rh-osbs/distributed-tracing-jaeger-operator-bundle@sha256:bf951190f9c71d3bf7776145be28dff46782efefee65525da845d5474a8b8bab]"
$ sqlite3 database/index.db 'SELECT * FROM channel_entry WHERE package_name="jaeger-product" # 'database/index.db' contains the catalog database post-add
15737|1.24-stable|jaeger-product|jaeger-operator.v1.24.1||0
15738|tech-preview|jaeger-product|jaeger-operator.v2.0.0-tp.1||0
15739|1.13-stable|jaeger-product|jaeger-operator.v1.13.2||0
15740|1.17-stable|jaeger-product|jaeger-operator.v1.17.10||0
15741|stable|jaeger-product|jaeger-operator.v1.26.0||0
15742|1.20-stable|jaeger-product|jaeger-operator.v1.20.5||0

/approve


getDefaultChannel := func(pkg string) sql.NullString {
// get defaultChannel before delete
rows, err := db.QueryContext(context.TODO(), `SELECT default_channel FROM package WHERE name = ?`, pkg)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: by convention context.Background() is used for tests.

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 8, 2021
Comment on lines 1820 to 1824
if _, err := tx.Exec(`UPDATE channel SET head_operatorbundle_name = NULL WHERE name = ? AND package_name = ? AND name IN (SELECT default_channel FROM package WHERE name = ?)`, channel, pkg, pkg); err != nil {
return err
}
if _, err := tx.Exec(`DELETE FROM channel WHERE name = ? AND package_name = ? AND head_operatorbundle_name = ?`, channel, pkg, bundle); err != nil {
return err
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At this point we know we're going to wipe this out using the default channel selection heuristic anyway, so why not leave the channel and default_channel alone? i.e. don't update/delete it at all.

Is there some edge case I'm missing?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I verified locally, it turns out we don't need to explicitly delete empty non-default channels, the heuristic handles that fine. We only need the head_operatorbundle_name nulling behavior around for the heuristic to pick a default channel when one isn't specified.

Signed-off-by: Ankita Thomas <ankithom@redhat.com>
Copy link
Member

@njhale njhale left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Oct 11, 2021
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Oct 11, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ankitathomas, njhale

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-merge-robot openshift-merge-robot merged commit c426f78 into operator-framework:master Oct 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants