Skip to content

Conversation

@grokspawn
Copy link
Contributor

@grokspawn grokspawn commented Sep 9, 2022

Signed-off-by: Jordan Keister jordan@nimblewidget.com

Description of the change:

add rendering of skipRange relationships to opm alpha render-graph and support an additional filter to allow users to limit results to a specific package in the catalog.

From usage:

Flags:
  -h, --help                  help for render-graph
      --minimum-edge string   the channel edge to be used as the lower bound of the set of edges composing the upgrade graph; default is to include all edges
  -p, --package-name string   a specific package name to filter output; default is to include all packages in reference

Global Flags:
      --skip-tls-verify   skip TLS certificate verification for container image registries while pulling bundles
      --use-http          use plain HTTP for container image registries while pulling bundles

Motivation for the change:
skipRange was supported in an earlier PR, but it started having complex interactions with this one, so I merged them here.
skipRange was a glaring inadequacy of the tool, and there have been multiple requestors for a package-filter option.

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
Copy link
Contributor

openshift-ci bot commented Sep 9, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: grokspawn

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 9, 2022
@codecov
Copy link

codecov bot commented Sep 9, 2022

Codecov Report

Merging #1023 (572c931) into master (a3c883e) will increase coverage by 0.27%.
The diff coverage is 74.32%.

❗ Current head 572c931 differs from pull request most recent head 0e37934. Consider uploading reports for the commit 0e37934 to get more accurate results

@@            Coverage Diff             @@
##           master    #1023      +/-   ##
==========================================
+ Coverage   51.68%   51.95%   +0.27%     
==========================================
  Files         102      102              
  Lines        9164     9215      +51     
==========================================
+ Hits         4736     4788      +52     
+ Misses       3520     3514       -6     
- Partials      908      913       +5     
Impacted Files Coverage Δ
alpha/declcfg/write.go 75.38% <74.32%> (+10.10%) ⬆️
alpha/veneer/semver/semver.go 60.00% <0.00%> (-0.50%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@timflannagan
Copy link
Member

Holding in case another review is needed.

/lgtm
/hold

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 9, 2022
@openshift-ci openshift-ci bot added lgtm Indicates that a PR is ready to be merged. and removed lgtm Indicates that a PR is ready to be merged. labels Sep 9, 2022
@grokspawn grokspawn force-pushed the render-graph-filter-operator branch from 9edb796 to 2a3cd63 Compare September 9, 2022 21:39
@grokspawn
Copy link
Contributor Author

Blech. Multiple filters got too complex, so collapsed other PR into this one.

@grokspawn grokspawn removed the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 9, 2022
@grokspawn
Copy link
Contributor Author

@cdjohnson try this one out. There was a bug where we would drop some nodes that I fixed. It makes for a slightly longer graph-text, but doesn't really impact the graph-pic at all.
I just didn't like the fact that it was incomplete. :)

@grokspawn grokspawn changed the title provide the capability to filter mermaid output to a single package name (feature) add skipRange support to opm alpha render-graph and provide the capability to filter mermaid output to a single package name Sep 9, 2022
@cdjohnson
Copy link
Contributor

I don't think the -p option is working right. It's assembling graphs from all sorts of other packages. Hence it's far too big to feed to mermaid.

To recreate:
opm alpha render-graph cp.icr.io/cpopen/ibm-operator-catalog:fbc-latest -p ibm-mq > mermaid.json

You'll see it's grabbing other packages:
example:
ibm-mq-v1.1-ibm-mq.v1.1.0["ibm-mq.v1.1.0"]-- "skipRange(>=1.0.0 <1.1.0)" --> ibm-mq-v1.1-ibm-aspera-hsts-operator.v1.0.0["ibm-aspera-hsts-operator.v1.0.0"]

If I constrain it manually by pointing at an FBC directory (via opm migrate with only the ibm-mq catalog.json in it (opm alpha render-graph ./catalog/configs/ibm-mq ...), then I get the right info. Here's what it looks like.

mq1

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 10, 2022
@grokspawn
Copy link
Contributor Author

grokspawn commented Sep 10, 2022

@cdjohnson You know ... it helps if you're considering the correct pool from which to get skipRange destination endpoints from ... I was pulling from the list of all bundles in the catalog, instead of the list of all other entities in the channel.
It appears to work for me now. 😄

I'll check more later.

@cdjohnson
Copy link
Contributor

Verified fixed!

return nil
}

// short-circuit where the minEdgeName is not in this channel
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if this is really a "short" short-circuit. It's a lot of work for a check isn't it? We're looping on c.Entries right below as well, can't any checking for the MinEdgeName just be combined into that loop?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It evaluates a single criterion across the range of entries in the channel in order to recognize a case where there is nothing pertinent in the channel (when the minimumEdge is specified).

I had attempted to include this in the logic of the larger loop, but it complicated the logic something fierce, as well as introducing inefficiencies like having to parse the skipRange twice for validation.

I'll take a look at it though to see if I can smooth it out. If you have any ideas, I'd love to hear them as well.

@grokspawn grokspawn force-pushed the render-graph-filter-operator branch 2 times, most recently from 572c931 to ab0ace3 Compare September 15, 2022 15:29
handle invalid SkipRange with output to stderr and ignoring the instance instead of dying
determine package of min filter to weed out mismatches

Signed-off-by: Jordan Keister <jordan@nimblewidget.com>
@grokspawn grokspawn force-pushed the render-graph-filter-operator branch from ab0ace3 to 0e37934 Compare September 15, 2022 15:35
@oceanc80
Copy link
Contributor

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Sep 19, 2022
@grokspawn grokspawn removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 20, 2022
@openshift-merge-robot openshift-merge-robot merged commit 0e53caf into master Sep 20, 2022
@grokspawn grokspawn deleted the render-graph-filter-operator branch September 20, 2022 18:08
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. lgtm Indicates that a PR is ready to be merged.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants