Skip to content

Generate package property during migration when no provided APIs.#742

Closed
benluddy wants to merge 1 commit into
operator-framework:masterfrom
benluddy:properties-migration-without-providedapis
Closed

Generate package property during migration when no provided APIs.#742
benluddy wants to merge 1 commit into
operator-framework:masterfrom
benluddy:properties-migration-without-providedapis

Conversation

@benluddy
Copy link
Copy Markdown
Contributor

@benluddy benluddy commented Aug 5, 2021

All entries beginning with schema version 9 should have at least one
property -- a generated package property. This did not occur for
catalog entries that had no provided APIs.

The existing migration is updated to fix the bug, and a new
migration is added to retroactively generate the package property for
databases that have already applied the existing migration.

@openshift-ci
Copy link
Copy Markdown
Contributor

openshift-ci Bot commented Aug 5, 2021

[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

Details 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 openshift-ci Bot requested review from njhale and timflannagan August 5, 2021 14:48
@openshift-ci openshift-ci Bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 5, 2021
@codecov
Copy link
Copy Markdown

codecov Bot commented Aug 5, 2021

Codecov Report

Merging #742 (eb91604) into master (3c2677e) will increase coverage by 0.09%.
The diff coverage is 84.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #742      +/-   ##
==========================================
+ Coverage   50.19%   50.29%   +0.09%     
==========================================
  Files         100      101       +1     
  Lines        8531     8530       -1     
==========================================
+ Hits         4282     4290       +8     
+ Misses       3421     3416       -5     
+ Partials      828      824       -4     
Impacted Files Coverage Δ
pkg/sqlite/migrations/009_properties.go 75.90% <81.81%> (+7.53%) ⬆️
...g/sqlite/migrations/013_ensure_package_property.go 85.71% <85.71%> (ø)

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 3c2677e...eb91604. Read the comment docs.

@kevinrizza
Copy link
Copy Markdown
Member

does editing the existing migration make sense? don't we need to handle dbs already past this version?

@benluddy
Copy link
Copy Markdown
Contributor Author

benluddy commented Aug 5, 2021

That's my question too. Properties are supposed to be opaque to the catalog, so it's also not great to go in and start generating properties after-the-fact as part of a later migration. Information is sort of implicitly lost after version 9, because we can no longer distinguish between the properties that were generated automatically as part of the migration versus a set of properties that the catalog maintainer may have specifically curated.

All entries beginning with schema version 9 should have at least one
property -- a generated package property. This did not occur for
catalog entries that had no provided APIs.

The existing migration is updated to fix the bug, and a new
migration is added to retroactively generate the package property for
databases that have already applied the existing migration.

Signed-off-by: Ben Luddy <bluddy@redhat.com>
@benluddy benluddy force-pushed the properties-migration-without-providedapis branch from b3bf33c to eb91604 Compare August 5, 2021 18:28
Up: func(ctx context.Context, tx *sql.Tx) error {
if _, err := tx.ExecContext(ctx, `
INSERT INTO properties(type, value, operatorbundle_name, operatorbundle_version, operatorbundle_path)
SELECT DISTINCT :property_type, json_object('packageName', channel_entry.package_name, 'version', operatorbundle.version), operatorbundle.name, operatorbundle.version, operatorbundle.bundlepath
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

do you need a CAST here?

@benluddy
Copy link
Copy Markdown
Contributor Author

With operator-framework/operator-lifecycle-manager#2291 merged, plus the existing package property inference feature, I don't think there is any pressure to make this change.

@benluddy benluddy closed this Aug 23, 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.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants