New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
bug 1827748: feature: opm (index|registry) prune command #243
bug 1827748: feature: opm (index|registry) prune command #243
Conversation
Hi @djzager. 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 Once the patch is verified, the new status will be reflected by the 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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great start. I do think that in order to be useful, it needs the ability to specify the channel with a package. The input could be a file of such pairs as CSV (comma-separated-values in this case), or this command could take positional args on the command line where the package and channel have some designated separator (maybe still a comma).
cmd/opm/index/filter.go
Outdated
if err := indexCmd.MarkFlagRequired("from-index"); err != nil { | ||
logrus.Panic("Failed to set required `from-index` flag for `index filter`") | ||
} | ||
indexCmd.Flags().StringSliceP("operators", "o", nil, "comma separated list of operators to filter") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe "packages" would work well here. That would especially prevent confusion around -o
being so heavily associated with output
in k8s and other contexts.
Though probably the best option is to replace this with a way to specify a list of package:channel pairs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that "packages" may be the least confusing. The problem for me is that I took these flags off of cmd/opm/index/delete.go
and so I'm hesitant to 1) mess with what already exists 2) change flags just for the filter command.
/ok-to-test |
a28056d
to
f193cfc
Compare
/retest |
/retest |
1 similar comment
/retest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm. There is a more SQL way to query which packages to delete from sqlite that might result in cleaner code. But not a biggie.
|
||
// get all the packages | ||
lister := sqlite.NewSQLLiteQuerierFromDb(db) | ||
packages, err := lister.ListPackages(context.TODO()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The other way to do this would be to write a query to select all packages with a NOT IN in place of selecting all packages. Then convert this list of package names into a comma separated string that can used in a single call tosqlite.NewSQLRemoverForPackages(dbLoader, pkg).
5ec19da
to
5876c1d
Compare
/hold I broke something. Will fix and add appropriate testcase. |
This makes it possible for a user to prune an index image or an operator database referencing a large collection of operator bundles.
/hold cancel |
/retest |
/lgtm Thanks for the wait @djzager! This looks good modulo some refactoring that we need to do across the entire indexer. Just so you know, we have to either file a BZ for this or wait for master to open (because we currently build upstream and downstream from this repo). |
@djzager: This pull request references Bugzilla bug 1827748, which is invalid:
Comment 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. |
/bugzilla refresh |
@djzager: This pull request references Bugzilla bug 1827748, which is invalid:
Comment 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. |
/bugzilla refresh |
@jmrodri: This pull request references Bugzilla bug 1827748, 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. |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: djzager, ecordell, mhrivnak 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 |
@djzager: All pull requests linked via external trackers have merged: operator-framework/operator-registry#243. Bugzilla bug 1827748 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. |
Description of the change:
This makes it possible for a user to take an index image or a database
referencing a large collection of operator bundles to filter down the
collection.
Motivation for the change:
Make it possible to scale down operator collections.
Reviewer Checklist
/docs