Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Index maintainers can export a package from an index #154

Merged
merged 1 commit into from
Jan 25, 2020
Merged

Index maintainers can export a package from an index #154

merged 1 commit into from
Jan 25, 2020

Conversation

gallettilance
Copy link
Member

@gallettilance gallettilance commented Jan 22, 2020 •
edited by ecordell
Loading

Description of the change:

Motivation for the change:

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

@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 Jan 22, 2020
@openshift-ci-robot openshift-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jan 22, 2020
exdx
exdx reviewed Jan 22, 2020
View reviewed changes
pkg/lib/indexer/interfaces.go Outdated Show resolved Hide resolved
exdx
exdx reviewed Jan 22, 2020
View reviewed changes
pkg/lib/indexer/interfaces.go Outdated Show resolved Hide resolved
@exdx
Copy link
Member

exdx commented Jan 22, 2020

I’m a little concerned about tying more code to sqlite just as we are trying to actively move to another backend -- but we have no choice, so we can just update the code later on no big deal.

@exdx
Copy link
Member

exdx commented Jan 22, 2020

/retest

ecordell
ecordell reviewed Jan 22, 2020
View reviewed changes
pkg/lib/indexer/indexer.go Outdated Show resolved Hide resolved
pkg/lib/indexer/indexer.go Show resolved Hide resolved
pkg/sqlite/image.go Outdated Show resolved Hide resolved
pkg/sqlite/image.go Outdated Show resolved Hide resolved
pkg/sqlite/query.go Outdated Show resolved Hide resolved
pkg/sqlite/query.go Outdated Show resolved Hide resolved
pkg/sqlite/query.go Outdated Show resolved Hide resolved
@exdx
Copy link
Member

exdx commented Jan 23, 2020

Added a long description of the export command as a form of documentation. Please let me know if anything looks amiss.

Export an operator from an index image into the appregistry format. 

This command will take an index image (specified by the --index option), parse it for the given operator (set by the --operator option) and export the operator metadata into an appregistry compliant format (a package.yaml file). 
This command requires access to docker or podman to complete successfully.

Note: the appregistry format is being deprecated in favor of the new index image and image bundle format.

@openshift-ci-robot openshift-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jan 23, 2020
@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 23, 2020
dinhxuanvu
dinhxuanvu reviewed Jan 23, 2020
View reviewed changes
Copy link
Member

@dinhxuanvu dinhxuanvu left a comment

Choose a reason for hiding this comment

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

A few comments. Looks good so far.

pkg/lib/bundle/exporter.go Outdated Show resolved Hide resolved
cmd/opm/index/export.go Show resolved Hide resolved
@openshift-ci-robot openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 23, 2020
@gallettilance gallettilance changed the title [WIP] Index maintainers can export a package from an index Index maintainers can export a package from an index Jan 23, 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 Jan 23, 2020
@gallettilance gallettilance force-pushed the index-to-appreg branch 2 times, most recently from 77242d2 to e67c332 Compare January 23, 2020 21:51
ecordell
ecordell reviewed Jan 23, 2020
View reviewed changes
cmd/opm/index/export.go Outdated Show resolved Hide resolved
pkg/lib/bundle/exporter.go Outdated Show resolved Hide resolved
pkg/lib/bundle/utils.go Outdated Show resolved Hide resolved
pkg/lib/indexer/indexer.go Show resolved Hide resolved
pkg/lib/indexer/indexer.go Outdated Show resolved Hide resolved
pkg/lib/indexer/indexer.go Outdated Show resolved Hide resolved
pkg/lib/indexer/indexer.go Show resolved Hide resolved
@exdx
Copy link
Member

exdx commented Jan 24, 2020

I added two tests for the refactored functions and also pulled some logic out of ExportFromIndex per @ecordell's suggestion. Tests pass so we have some coverage. One test queries the DB directly and we didn't have time to stub it out, so I had to check in an index sqlite DB into the code. Obviously this is not ideal but in the interest of test coverage I thought it would be ok - sqlite is going away in the near future regardless. Feel free to refactor further or revert the commit as needed.

@gallettilance gallettilance force-pushed the index-to-appreg branch 2 times, most recently from 3545592 to 50547d3 Compare January 24, 2020 01:32
ecordell
ecordell reviewed Jan 24, 2020
View reviewed changes
pkg/lib/bundle/utils.go Outdated Show resolved Hide resolved
@gallettilance gallettilance force-pushed the index-to-appreg branch 2 times, most recently from b7dbb68 to 9f37323 Compare January 24, 2020 13:33
@exdx
Copy link
Member

exdx commented Jan 24, 2020

/lgtm

@dinhxuanvu
Copy link
Member

/retest

@exdx
Copy link
Member

exdx commented Jan 24, 2020

/lgtm

@openshift-ci-robot openshift-ci-robot added lgtm Indicates that a PR is ready to be merged. and removed lgtm Indicates that a PR is ready to be merged. labels Jan 24, 2020
exdx
exdx reviewed Jan 24, 2020
View reviewed changes
opm-test.sh Show resolved Hide resolved
@exdx
Copy link
Member

exdx commented Jan 24, 2020

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 24, 2020
@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Jan 24, 2020
ecordell
ecordell approved these changes Jan 24, 2020
View reviewed changes
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.

/lgtm

@openshift-ci-robot openshift-ci-robot added lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jan 24, 2020
@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Jan 24, 2020
@exdx
Copy link
Member

exdx commented Jan 24, 2020

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 24, 2020
@dinhxuanvu
Copy link
Member

/lgtm

@openshift-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dinhxuanvu, ecordell, exdx, gallettilance

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:
  • OWNERS [dinhxuanvu,ecordell]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

1 similar comment
@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@gallettilance
Copy link
Member Author

/test e2e-aws

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-merge-robot openshift-merge-robot merged commit 2b33801 into operator-framework:master Jan 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@benluddy benluddy benluddy left review comments

@dinhxuanvu dinhxuanvu dinhxuanvu left review comments

@exdx exdx exdx left review comments

@ecordell ecordell ecordell approved these changes

@jpeeler jpeeler Awaiting requested review from jpeeler

Assignees

@ecordell ecordell

@dinhxuanvu dinhxuanvu

@exdx exdx

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. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@gallettilance @exdx @dinhxuanvu @openshift-bot @openshift-ci-robot @ecordell @benluddy @openshift-merge-robot