operator-registry icon indicating copy to clipboard operation
operator-registry copied to clipboard

feat(opm): Create multiple files during migration

Open anik120 opened this issue 4 years ago • 8 comments

Description of the change:

In this PR, the opm migrate command is enhanced to create multiple plaintext files, one each for each package, bundles and channels intead of a single file for each package.

Motivation for the change:

When plaintext files are loaded onto memory, they are able to be loaded even if the metadata is spread across multiple files (instead of just one file). Splitting the metadata logically in a way this PR does makes the files more human consumable.

Reviewer Checklist

  • [ ] Implementation matches the proposed design, or proposal is updated to match implementation
  • [ ] Sufficient unit test coverage
  • [ ] Sufficient end-to-end test coverage
  • [ ] Docs updated or added to /docs
  • [ ] Commit messages sensible and descriptive

anik120 avatar Oct 11 '21 21:10 anik120

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: anik120 To complete the pull request process, please assign dinhxuanvu after the PR has been reviewed. You can assign the PR to them by writing /assign @dinhxuanvu in a comment when ready.

The full list of commands accepted by this bot can be found 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-ci[bot] avatar Oct 11 '21 21:10 openshift-ci[bot]

Codecov Report

Merging #815 (512ecc3) into master (7af20e0) will decrease coverage by 0.02%. The diff coverage is 44.00%.

@@            Coverage Diff             @@
##           master     #815      +/-   ##
==========================================
- Coverage   51.04%   51.03%   -0.02%     
==========================================
  Files         103      103              
  Lines        9050     9071      +21     
==========================================
+ Hits         4620     4629       +9     
- Misses       3553     3559       +6     
- Partials      877      883       +6     
Impacted Files Coverage Δ
alpha/action/migrate.go 55.17% <44.00%> (-6.99%) :arrow_down:

... and 1 file with indirect coverage changes

codecov[bot] avatar Oct 11 '21 21:10 codecov[bot]

It feels like this is something we should probably have a veneer for, opm render also has this problem with not splitting out object types for readability.

@ankitathomas while approaching this PR, I'd like to not conflate all of the issues you bring up here. This change is independent of any other effort, for the sake of simplicity, all this PR approaches is, "it'll be useful to have the output of opm migrate organized a bit more". If there are any reasons why having that convenience of organized output will be detrimental to users, I'm happy to hear those and start another discussion.

anik120 avatar Oct 18 '21 18:10 anik120

I'm inclined to put a /hold on this PR, mainly because I think changing migrate right now is premature.

I don't want us adding more complexity to it (e.g. a new flag) that has the potential to dictate the direction we take migrate in the future unless we're absolutely positive that the feature (and the added complexity) is actually being requested by the pipeline or a group of representative operator authors.

joelanford avatar Oct 20 '21 02:10 joelanford

unless we're absolutely positive that the feature (and the added complexity) is actually being requested by the pipeline or a group of representative operator authors.

@joelanford AFAIK no pipeline/group of representative operator authors requested migrate to begin with. The migrate that's in there now was cooked up by us to begin with, and this PR is just enhancing the soup a little bit.

I don't want us adding more complexity to it (e.g. a new flag) that has the potential to dictate the direction we take migrate in the future

If anything the flag is just to appease both set of requests, "I want the output organised" and "I want the output as single files". Bottom line, I being the only user of the command at this point (as a result of prototyping migrating a catalog), I'd rather have it only as organized output than giant single files.

anik120 avatar Oct 20 '21 18:10 anik120

@anik120: PR needs rebase.

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.

openshift-merge-robot avatar Aug 06 '22 11:08 openshift-merge-robot

We wrote a simple script to expand the catalog.json after a migrate into schema only json files but would be great to have this built into opm migrate and/or opm render. Example catalog.json expanded output for aimanager-operator package with two channels and one bundle in each:

olm.bundle-aimanager-operator.v1.0.0.json
olm.bundle-aimanager-operator.v2.0.0.json
olm.channel-v1.0.json
olm.channel-v2.0.json
olm.package-aimanager-operator.json

jdockter avatar Aug 30 '22 21:08 jdockter

Here's a one-liner: cat catalog.json | jq -sr '.[] | @sh "cat >\(.schema)-\(.name).json <<\\END",.,"END"' | sh

cdjohnson avatar Sep 01 '22 03:09 cdjohnson

closing PR as stale. Please re-open if it's still important.

perdasilva avatar Feb 19 '24 13:02 perdasilva