Skip to content

generify mermaid channel writer for all declcfg #987

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
Merged
merged 1 commit into from
Jul 26, 2022
Merged

generify mermaid channel writer for all declcfg #987

merged 1 commit into from
Jul 26, 2022

Conversation

@grokspawn
Copy link
Contributor

@grokspawn grokspawn commented Jul 7, 2022 •
edited
Loading

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

Description of the change:
Provide a generic mechanism by which any FBC can have access to the mermaid channel graph writer.
Closes #973
Now states in output that skipRange edges are not presently supported, and sorts by package name and then channel name

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 openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 7, 2022
@codecov
Copy link

codecov bot commented Jul 7, 2022 •
edited
Loading

Codecov Report

Merging #987 (5b8e745) into master (05b1edc) will increase coverage by 0.19%.
The diff coverage is 100.00%.

❗ Current head 5b8e745 differs from pull request most recent head b36659d. Consider uploading reports for the commit b36659d to get more accurate results

@@            Coverage Diff             @@
##           master     #987      +/-   ##
==========================================
+ Coverage   52.64%   52.84%   +0.19%     
==========================================
  Files         104      104              
  Lines        9444     9445       +1     
==========================================
+ Hits         4972     4991      +19     
+ Misses       3539     3525      -14     
+ Partials      933      929       -4
Impacted Files Coverage Δ
alpha/veneer/semver/semver.go 61.62% <ø> (+2.80%) ⬆️
alpha/declcfg/write.go 82.29% <100.00%> (+9.27%) ⬆️

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 05b1edc...b36659d. Read the comment docs.

@grokspawn grokspawn force-pushed the gen_fbc_writer branch 2 times, most recently from 4cc993b to 449f4b3 Compare July 11, 2022 15:45
@grokspawn grokspawn changed the title WIP: generify mermaid channel writer for all declcfg generify mermaid channel writer for all declcfg Jul 11, 2022
@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 Jul 11, 2022
@grokspawn grokspawn requested review from awgreene and joelanford July 11, 2022 15:58
dinhxuanvu
dinhxuanvu reviewed Jul 18, 2022
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.

Looking good. Just one nit.

joelanford
joelanford reviewed Jul 19, 2022
View reviewed changes
exdx
exdx approved these changes Jul 19, 2022
View reviewed changes
Copy link
Contributor

@exdx exdx left a comment

Choose a reason for hiding this comment

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

Nice!

@grokspawn grokspawn force-pushed the gen_fbc_writer branch 2 times, most recently from 147c49a to dc3ad57 Compare July 19, 2022 18:17
dinhxuanvu
dinhxuanvu reviewed Jul 19, 2022
View reviewed changes
dinhxuanvu
dinhxuanvu reviewed Jul 19, 2022
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.

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jul 19, 2022
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Jul 19, 2022
joelanford
joelanford reviewed Jul 20, 2022
View reviewed changes
dinhxuanvu
dinhxuanvu approved these changes Jul 21, 2022
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.

/approve

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jul 21, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dinhxuanvu, exdx, 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 Jul 21, 2022
@grokspawn
relocate mermaid channel writer
b36659d 
Signed-off-by: Jordan Keister <jordan@nimblewidget.com>
@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 23, 2022
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 25, 2022
awgreene
awgreene reviewed Jul 26, 2022
View reviewed changes
Copy link
Member

@awgreene awgreene left a comment

Choose a reason for hiding this comment

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

/lgtm

cmd/opm/alpha/veneer/semver.go
write = declcfg.WriteYAML
case "mermaid":
write = semver.MermaidChannelWriter
write = declcfg.WriteMermaidChannels
Copy link
Member

@awgreene awgreene Jul 26, 2022

Choose a reason for hiding this comment

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

Non-blocking: Hmm, a write function that takes a config variable to determine output format might be nice.

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jul 26, 2022
@openshift-bot
Copy link
Contributor

openshift-bot commented Jul 26, 2022

/retest-required

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

@openshift-merge-robot openshift-merge-robot merged commit 524a11c into master Jul 26, 2022
@grokspawn grokspawn deleted the gen_fbc_writer branch July 26, 2022 16:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@joelanford joelanford joelanford left review comments

@awgreene awgreene awgreene left review comments

@dinhxuanvu dinhxuanvu dinhxuanvu approved these changes

@gallettilance gallettilance Awaiting requested review from gallettilance

@timflannagan timflannagan Awaiting requested review from timflannagan

+1 more reviewer

@exdx exdx exdx approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@awgreene awgreene

@dinhxuanvu dinhxuanvu

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

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Feature Request: Create a generic writer for veneers

8 participants

@grokspawn @openshift-bot @joelanford @awgreene @dinhxuanvu @exdx @openshift-merge-robot