Skip to content

Conversation

@estroz
Copy link
Member

@estroz estroz commented Jul 23, 2021

Description of the change: use --include-file,-i to include a set of packages, channels, and/or versions in opm alpha diff output.

Motivation for the change: you may want to explicitly include an upgrade graph (especially in heads-only mode) in your diff, ex. when you have a specific version requirement.

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

TODO:

  • tests
  • code comments

/kind feature

@openshift-ci openshift-ci bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. kind/feature Categorizes issue or PR as related to a new feature. labels Jul 23, 2021
@openshift-ci openshift-ci bot requested review from ankitathomas and ecordell July 23, 2021 05:57
@codecov
Copy link

codecov bot commented Jul 23, 2021

Codecov Report

Merging #716 (9048884) into master (a3253eb) will decrease coverage by 0.26%.
The diff coverage is 29.59%.

❗ Current head 9048884 differs from pull request most recent head 836c742. Consider uploading reports for the commit 836c742 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master     #716      +/-   ##
==========================================
- Coverage   50.90%   50.64%   -0.27%     
==========================================
  Files         102      102              
  Lines        8753     8846      +93     
==========================================
+ Hits         4456     4480      +24     
- Misses       3458     3522      +64     
- Partials      839      844       +5     
Impacted Files Coverage Δ
alpha/declcfg/diff_include.go 31.91% <3.17%> (-58.72%) ⬇️
alpha/declcfg/diff.go 77.14% <45.45%> (-2.17%) ⬇️
alpha/action/diff.go 75.86% <91.66%> (+11.15%) ⬆️

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 a3253eb...836c742. Read the comment docs.

@estroz estroz mentioned this pull request Jul 28, 2021
5 tasks
@estroz estroz force-pushed the feat/opm-diff-include branch from b1cc546 to 5ca6400 Compare August 6, 2021 06:01
@estroz estroz changed the title [WIP] feat(opm diff): include packages, channels, and versions in diff output feat(opm diff): include packages, channels, and versions in diff output Aug 6, 2021
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 6, 2021
@estroz
Copy link
Member Author

estroz commented Aug 6, 2021

I still need to add more tests, but this is ready for review.

/cc @joelanford @kevinrizza

@openshift-ci openshift-ci bot requested review from joelanford and kevinrizza August 6, 2021 06:03
@openshift-ci openshift-ci bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 20, 2021
@estroz estroz force-pushed the feat/opm-diff-include branch from 5ca6400 to 3cef8db Compare August 21, 2021 03:28
@openshift-ci openshift-ci bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 21, 2021
Copy link
Contributor

@ankitathomas ankitathomas left a comment

Choose a reason for hiding this comment

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

Great work! This needs some refactoring over the olm.channel schema added.

EOF
opm alpha diff registry.org/my-catalog:def456 -i include.yaml -o yaml > my-catalog-index/index.yaml
# FINALLY:
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks like it's missing a render step for including the old catalog in the declarative config directory.

Copy link
Member Author

@estroz estroz Sep 8, 2021

Choose a reason for hiding this comment

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

Done. the old catalog shouldn't be in the DC dir, just the diff in my-catalog-index/index.yaml.

@openshift-ci openshift-ci bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 5, 2021
@estroz estroz force-pushed the feat/opm-diff-include branch from 3cef8db to 49ffe71 Compare September 8, 2021 21:59
@openshift-ci openshift-ci bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 8, 2021
@estroz estroz requested a review from ankitathomas September 8, 2021 22:13
@estroz estroz force-pushed the feat/opm-diff-include branch from 49ffe71 to 8c33405 Compare September 8, 2021 23:04
Signed-off-by: Eric Stroczynski <ericstroczynski@gmail.com>
@estroz estroz force-pushed the feat/opm-diff-include branch from 8c33405 to 836c742 Compare September 9, 2021 19:20
@estroz
Copy link
Member Author

estroz commented Sep 9, 2021

/override go-apidiff / go-apidiff

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Sep 9, 2021

@estroz: /override requires a failed status context or a job name to operate on.
The following unknown contexts were given:

  • go-apidiff / go-apidiff

Only the following contexts were expected:

  • DCO
  • build
  • e2e
  • sanity
  • tide
  • unit

In response to this:

/override go-apidiff / go-apidiff

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.

@estroz
Copy link
Member Author

estroz commented Sep 10, 2021

@joelanford we should probably ignore the alpha package when running apidiff.

@joelanford
Copy link
Member

That would be a new feature for apidiff. Right now it considers anything that is technically possible to be imported by another project as a public API.

@estroz
Copy link
Member Author

estroz commented Sep 10, 2021

Ah bummer. Since the test is not required, prow should merge this when lgtm'd anyway.

@kevinrizza
Copy link
Member

/approve

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Sep 15, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: estroz, kevinrizza

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 15, 2021
Copy link
Member

@joelanford joelanford 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 openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Sep 17, 2021
@openshift-merge-robot openshift-merge-robot merged commit 3880486 into operator-framework:master Sep 17, 2021
@estroz estroz deleted the feat/opm-diff-include branch September 17, 2021 20:19
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. kind/feature Categorizes issue or PR as related to a new feature. 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