Skip to content

Conversation

@djzager
Copy link
Contributor

@djzager djzager commented Apr 3, 2020

Enhancement request to add prune subcommand to opms registry and
index commands.

@djzager djzager changed the title feat: opm package filtering WIP: feat: opm package filtering Apr 3, 2020
@openshift-ci-robot openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 3, 2020
@djzager
Copy link
Contributor Author

djzager commented Apr 3, 2020

@ecordell @mhrivnak

@djzager djzager changed the title WIP: feat: opm package filtering feat: opm package filtering Apr 6, 2020
@openshift-ci-robot openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 6, 2020
@djzager
Copy link
Contributor Author

djzager commented Apr 6, 2020

This enhancement request is implemented in operator-framework/operator-registry#251

Copy link
Member

@ecordell ecordell left a comment

Choose a reason for hiding this comment

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

/approve

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.

Thanks for adding this!

I just had one primary question:

#### New Commands

A `filter` subcommand will be added to both the `index` and `registry` command
mimmicking the behavior of `rm` in all but one significant way. The `operators`
Copy link
Member

@njhale njhale Apr 6, 2020

Choose a reason for hiding this comment

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

if it's a new subcommand, could it not use packages in both instances? Alternatively, the package list could be the positional parameters of the command, rather than a named parameter; e.g. opm index filter <bundle-img> <pkg-0> <pkg-1> ... <pkg-n>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My only struggle with this idea is wanting consistency. For example, when you remove operators from an index by saying opm index rm --from-index <index-img> --operators <op1>,<op2> and it seems fair for a user to think prune would look something like opm index prune --from-index <index-img> --operators <op1>,<op2>.

It may be that packages is really most appropriate for all of opm (index|registry) (rm|prune) but I'm hesitant to do that for all of opm (index|registry) prune without consensus. WDYT @ecordell ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

I like being consistent, though IMO the existing commands are probably not following typical UX conventions; e.g no primary positional argument, required options, etc.

@djzager djzager changed the title feat: opm package filtering feat: opm package pruning Apr 7, 2020
@kevinrizza
Copy link
Member

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 7, 2020
Comment on lines 69 to 72
A `prune` subcommand will be added to both the `index` and `registry` command
mimmicking the behavior of `rm` in all but one significant way. The `operators`
flag (for `index`es) and `packages` flag (for `registry`es) will represent the
packages to be kept in the index or registry.
Copy link
Member

Choose a reason for hiding this comment

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

will these both be represented by a packages option? e.g. opm index prune --packages <pkgs> ... and opm registry prune --packages <pkgs> ...

Enhancement request to add `prune` subcommand to `opm`s `registry` and
`index` commands.
@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Apr 8, 2020
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-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 13, 2020
@ecordell ecordell merged commit 4fe91a7 into operator-framework:master Apr 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

lgtm Indicates that a PR is ready to be merged.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants