-
Notifications
You must be signed in to change notification settings - Fork 260
initial working viz of ignored v0 edges #1744
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.
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
initial working viz of ignored v0 edges #1744
Conversation
Signed-off-by: grokspawn <[email protected]>
Skipping CI for Draft Pull Request. |
1d01c9b
to
9f8c791
Compare
0e8cfc1
to
7d3cf25
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1744 +/- ##
==========================================
+ Coverage 55.17% 55.24% +0.06%
==========================================
Files 136 136
Lines 15918 15971 +53
==========================================
+ Hits 8783 8823 +40
- Misses 5982 5993 +11
- Partials 1153 1155 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
pkgBuilder.WriteString(fmt.Sprintf(" %%%% channel %q\n", filteredChannel.Name)) | ||
pkgBuilder.WriteString(fmt.Sprintf(" subgraph %s[%q]\n", channelID, filteredChannel.Name)) | ||
fmt.Fprintf(pkgBuilder, " %%%% channel %q\n", filteredChannel.Name) | ||
fmt.Fprintf(pkgBuilder, " subgraph %s[%q]\n", channelID, filteredChannel.Name) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
bin/opm alpha render-graph quay.io/operatorhubio/catalog:latest|head 2>/dev/null edit: the above command is wrong, stderr is correctly used, but maybe we should give an example in comments of redirecting as needed:
|
@bentito I think trying to graph an entire catalog will not function properly (though I'd love to see the warnings readout). You would want to limit to a single package, and possibly a lower version range via flags. I generated customized FBC here, based on a semver catalog template expansion. (Original report here ) The only missing detail is that, since OPM handles FBC as a hierarchical filesystem, I had to drop the FBC into a directory to manipulate it. For e.g.: mkdir /tmp/c4
./bin/opm alpha render-template semver reproducer-semver.yaml > /tmp/c4/catalog.json
./bin/opm alpha render-graph /tmp/c4 |
lgtm |
/approve |
[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 |
cmd/opm/alpha/render-graph/cmd.go
Outdated
} | ||
cmd.Flags().StringVar(&minEdge, "minimum-edge", "", "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") | ||
cmd.Flags().StringVarP(&specifiedPackageName, "package-name", "p", "", "a specific package name to filter output; default is to include all packages in reference") | ||
cmd.Flags().BoolVar(&includeV0Semantics, "include-v0-semantics", false, "whether to indicate OLMv0 semantics in the output; default is to simply represent the upgrade graph") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My only concern is with the flag name and description
Could we try to clarify better
If I am adding the flag than I am including that already right?
What does it acctually means v0-semantics?
Maybe:
cmd.Flags().BoolVar(&includeV0Semantics, "include-v0-semantics", false, "whether to indicate OLMv0 semantics in the output; default is to simply represent the upgrade graph") | |
cmd.Flags().BoolVar(&includeV0Semantics, "--olmv0-semantics", false, "apply OLM v0 upgrade-graph semantics; mark skipped bundles and ignore edges from them when computing paths.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR is the first pass of what I expect will be additional efforts to make how v0 uses upgrade graphs more transparent. I deliberately left the flag very general as a result. This is all in an alpha command chain, so we have a lot of freedom to change the flags in the future as the support improves.
cmd/opm/alpha/render-graph/cmd.go
Outdated
render action.Render | ||
minEdge string | ||
specifiedPackageName string | ||
includeV0Semantics bool |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about we add a doc here to explain why it is required?
What does it means?
Maybe:
includeV0Semantics bool | |
// includeV0Semantics show and compute the upgrade graph the way OLM v0 behaves: if a bundle is | |
// marked as skipped (via skips or skipRange), ignore any upgrade edges coming from that bundle. | |
// This can reveal why some versions get “stuck.” | |
includeV0Semantics bool |
cmd/opm/alpha/render-graph/cmd.go
Outdated
} | ||
cmd.Flags().StringVar(&minEdge, "minimum-edge", "", "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") | ||
cmd.Flags().StringVarP(&specifiedPackageName, "package-name", "p", "", "a specific package name to filter output; default is to include all packages in reference") | ||
cmd.Flags().BoolVar(&includeV0Semantics, "include-v0-semantics", false, "whether to indicate OLMv0 semantics in the output; default is to simply represent the upgrade graph") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My nit is s/include/use/
for the name of the flag/variable, etc.
"Include" implies multiple $things could be included, all contributing. Whereas "use" has more of a mutual exclusive feel, which I think is more of what we're going for? I also notice that the actual mermaid renderer uses "use" terminology, so this would have better overall alignment as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’d prefer to use that as well.
Sorry for being so nitpicky, but could you please include “ring a bell”?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I updated to make the flag naming consistent across the front- & back-ends.
I don't understand the "ring a bell" reference.
7d3cf25
to
b35b44c
Compare
Signed-off-by: grokspawn <[email protected]>
371dd97
to
f1c21be
Compare
/lgtm |
2629279
into
operator-framework:master
Description of the change:
This is a first pass in making OLMv0 upgrade graph mechanics visible in rendered graphs, adding to existing
olm.deprecations
indication rendering.Bundle versions which are skipped in a graph are outlined in red and have all their contributory edges changed to red, dashed arrows to indicate that they are ignored when calculating upgrades, for e.g.

in this graph, cert-manager-operator.v1.15.1 is skipped, so any graph updates by this version are similarly ignored. This means that users cannot upgrade from v1.15.0 to v1.15.1.
These features are controlled by a new
draw-v0-semantics
flag (defaults off).By default, render-graph displays simple upgrade graph mechanics, which aligns to OLMv1 functionality.
Motivation for the change:
Answering user questions recently, it became apparent that OLMv0 interpretation of an upgrade graph includes some interpretation mechanics which aren't well-communicated. These mechanics can result in some user stranding on operator versions because of ignored graph contributions.
Reviewer Checklist
/docs