Skip to content

feat(diff): change default behavior to prune by omission #819

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
Nov 5, 2021
Merged

feat(diff): change default behavior to prune by omission #819

merged 1 commit into from
Nov 5, 2021

Conversation

@estroz
Copy link
Member

@estroz estroz commented Nov 2, 2021

Description of the change: change the behavior of opm alpha diff to only include stuff from the include file by default, and optionally act additively with --include-additive

Motivation for the change: it is counterintuitive to include additively by default

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 requested review from exdx and gallettilance November 2, 2021 19:11
@codecov
Copy link

codecov bot commented Nov 2, 2021 •
edited
Loading

Codecov Report

Merging #819 (21fd210) into master (d5522b3) will increase coverage by 0.31%.
The diff coverage is 69.56%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #819      +/-   ##
==========================================
+ Coverage   51.08%   51.39%   +0.31%     
==========================================
  Files         103      103              
  Lines        9056     9020      -36     
==========================================
+ Hits         4626     4636      +10     
+ Misses       3553     3510      -43     
+ Partials      877      874       -3
Impacted Files Coverage Δ
alpha/declcfg/diff.go 77.48% <66.66%> (+0.02%) ⬆️
alpha/action/diff.go 77.04% <100.00%> (+0.38%) ⬆️
pkg/registry/csv.go 71.42% <0.00%> (-5.45%) ⬇️
pkg/registry/registry_to_model.go 59.72% <0.00%> (+0.02%) ⬆️
alpha/action/render.go 64.14% <0.00%> (+0.26%) ⬆️
alpha/declcfg/diff_include.go 61.32% <0.00%> (+33.01%) ⬆️

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 d5522b3...21fd210. Read the comment docs.

@estroz estroz mentioned this pull request Nov 4, 2021
Closed
@estroz estroz changed the title feat(declcfg): add IncludeAdditively option to DiffGenerator feat(diff): change default behavior to prune Nov 4, 2021
@estroz estroz changed the title feat(diff): change default behavior to prune feat(diff): change default behavior to prune by omission Nov 4, 2021
@estroz
Copy link
Member Author

estroz commented Nov 5, 2021

/cc @ankitathomas @dinhxuanvu

kevinrizza
kevinrizza reviewed Nov 5, 2021
View reviewed changes
Copy link
Member

@kevinrizza kevinrizza left a comment

Choose a reason for hiding this comment

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

Some small nits, lgtm otherwise

cmd/opm/alpha/diff/cmd.go
- If in latest mode (old-refs is specified), a diff between old-refs and new-refs is added to the output.
- If --include-file is set, items from that file will be added to the diff:
- If --include-additive is false (the default), a diff will be generated only on those objects, depending on the mode.
- If --include-additive is true, the diff will contain included objects, plus those added by the mode's invocation.
Copy link
Member

@kevinrizza kevinrizza Nov 5, 2021

Choose a reason for hiding this comment

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

is it possible to include some optional help text with example calls?

Copy link
Member Author

@estroz estroz Nov 5, 2021

Choose a reason for hiding this comment

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

Done.

alpha/declcfg/diff.go
latestMode := !headsOnlyMode
isInclude := len(g.Includer.Packages) != 0

switch {
Copy link
Member

@kevinrizza kevinrizza Nov 5, 2021

Choose a reason for hiding this comment

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

Is a switch statement better than a nested if statement here? Worrying about the default case running or not seems like it would be easy to break on further refactor

Copy link
Member Author

@estroz estroz Nov 5, 2021

Choose a reason for hiding this comment

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

An if statement would look more confusing, and a switch statement supports future modes more intuitively.

alpha/action/diff_test.go
},
},
},
IncludeAdditively: true,
Copy link
Member

@kevinrizza kevinrizza Nov 5, 2021

Choose a reason for hiding this comment

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

Do we want to add some test cases where IncludeAdditively is false for explicit output comparison?

Copy link
Member Author

@estroz estroz Nov 5, 2021

Choose a reason for hiding this comment

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

The tests I added elsewhere cover this case. These are mostly to ensure input is parsed and output is rendered correctly with real files.

@estroz
feat(diff): change default behavior to prune by omission
21fd210 
Signed-off-by: Eric Stroczynski <ericstroczynski@gmail.com>
@estroz estroz requested a review from kevinrizza November 5, 2021 18:29
@kevinrizza
Copy link
Member

kevinrizza commented Nov 5, 2021

/approve

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 5, 2021
dinhxuanvu
dinhxuanvu approved these changes Nov 5, 2021
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 Nov 5, 2021
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Nov 5, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dinhxuanvu, 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:
  • OWNERS [dinhxuanvu,kevinrizza]

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

@openshift-merge-robot openshift-merge-robot merged commit 29cd8e3 into operator-framework:master Nov 5, 2021
@estroz
Copy link
Member Author

estroz commented Nov 6, 2021

Thanks!

@estroz estroz deleted the feat/diff-prune branch November 6, 2021 05:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dinhxuanvu dinhxuanvu dinhxuanvu approved these changes

@exdx exdx Awaiting requested review from exdx

@gallettilance gallettilance Awaiting requested review from gallettilance

@ankitathomas ankitathomas Awaiting requested review from ankitathomas

@kevinrizza kevinrizza Awaiting requested review from kevinrizza

Assignees

@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.

4 participants

@estroz @kevinrizza @dinhxuanvu @openshift-merge-robot