refactor: Successor getting with separation of concerns by TerryHowe · Pull Request #1443 · oras-project/oras

TerryHowe · 2024-07-11T16:44:04Z

What this PR does / why we need it:

The PrintSuccessorStatus method was complicated with the function passed in and it didn't show the best separation of concerns. The print module ideally wouldn't know about a fetcher for example.

codecov · 2024-07-11T16:51:06Z

Codecov Report

Attention: Patch coverage is 85.59322% with 17 lines in your changes missing coverage. Please review.

Project coverage is 83.43%. Comparing base (c31d120) to head (d9e22e8).

Files	Patch %	Lines
cmd/oras/internal/display/status/text.go	80.76%	2 Missing and 3 partials ⚠️
cmd/oras/root/cp.go	28.57%	3 Missing and 2 partials ⚠️
internal/testutils/fetcher.go	83.33%	3 Missing ⚠️
cmd/oras/internal/display/status/tty.go	90.00%	2 Missing ⚠️
cmd/oras/root/attach.go	66.66%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1443      +/-   ##
==========================================
+ Coverage   83.29%   83.43%   +0.13%     
==========================================
  Files         110      112       +2     
  Lines        3892     3936      +44     
==========================================
+ Hits         3242     3284      +42     
- Misses        426      428       +2     
  Partials      224      224

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

TerryHowe

/lgtm

qweeah · 2024-07-17T06:21:32Z

@TerryHowe We have changed the behavior of Println. Print error will now be returned as terminal errors. Can you help review the change, thanks.

I also tracked to-do fixes in other places in #1446

TerryHowe

/lgtm

I cannot approve this, but looks good

shizhMSFT

LGTM with suggestion. This PR also requires improved code testing. I'd suggest adding some unit tests.

TerryHowe · 2024-07-22T15:15:43Z

I was kind of hoping to do this as a follow up, but to increase test coverage, I moved descriptor.Successors to graph.FilteredSuccessors since it is just a specialization of graph.Successors and the framework to test it was in graph_test.go.

TerryHowe · 2024-07-22T15:24:00Z

There is a bit of a chicken/egg situation here. The whole reason I went down this path of refactoring successors was to create a tty handler and once the tty handler was created, the code would be way more testable. I need to think about this.

qweeah

LGTM, missing lines in tty.go and text.go can be covered by unit tests though.

qweeah · 2024-08-06T05:55:28Z

@TerryHowe Can you help add unit test to cover missing lines in cmd/oras/internal/display/status/text.go? Thanks

Signed-off-by: Terry Howe <terrylhowe@gmail.com>

Signed-off-by: Billy Zha <jinzha1@microsoft.com>

Signed-off-by: Terry Howe <terrylhowe@gmail.com>

Signed-off-by: Billy Zha <jinzha1@microsoft.com>

qweeah · 2024-08-09T15:54:35Z

@TerryHowe You have chosen the hard way but you have done it good! Thanks for the contribution. Added several test to cover more lines. PTAL.

TerryHowe · 2024-08-09T16:11:29Z

@TerryHowe You have chosen the hard way but you have done it good! Thanks for the contribution. Added several test to cover more lines. PTAL.

Thanks for working with me on this and all the others!

LGTM

TerryHowe requested review from SteveLasker, qweeah, sajayantony and shizhMSFT as code owners July 11, 2024 16:44

qweeah reviewed Jul 12, 2024

View reviewed changes

Comment thread internal/descriptor/descriptor.go Outdated

TerryHowe force-pushed the get-successors branch from 46084e7 to 5cf03b7 Compare July 12, 2024 15:14

TerryHowe commented Jul 15, 2024

View reviewed changes

shizhMSFT reviewed Jul 16, 2024

View reviewed changes

Comment thread internal/descriptor/descriptor.go Outdated

Comment thread internal/descriptor/descriptor.go Outdated

Comment thread cmd/oras/internal/display/status/utils.go Outdated

Comment thread cmd/oras/internal/display/status/text.go Outdated

Comment thread cmd/oras/root/cp.go Outdated

TerryHowe commented Jul 17, 2024

View reviewed changes

Comment thread cmd/oras/internal/display/metadata/text/push_test.go Outdated

TerryHowe commented Jul 17, 2024

View reviewed changes

shizhMSFT approved these changes Jul 22, 2024

View reviewed changes

Comment thread cmd/oras/internal/output/print.go Outdated

TerryHowe force-pushed the get-successors branch from be31fa7 to 06ba573 Compare July 22, 2024 15:05

TerryHowe force-pushed the get-successors branch from 06ba573 to d560884 Compare July 22, 2024 15:18

qweeah approved these changes Jul 24, 2024

View reviewed changes

TerryHowe force-pushed the get-successors branch 6 times, most recently from 4c50b9c to 5d5d6ee Compare July 26, 2024 21:59

TerryHowe force-pushed the get-successors branch 2 times, most recently from 54f841f to 12c4d07 Compare August 3, 2024 14:33

TerryHowe force-pushed the get-successors branch from 8e97c6b to 270ad71 Compare August 7, 2024 12:45

TerryHowe and others added 2 commits August 8, 2024 19:16

refactor: Successor getting with separation of concerns

0b2cc3b

Signed-off-by: Terry Howe <terrylhowe@gmail.com>

test(unit): increase coverage

7fcc381

Signed-off-by: Billy Zha <jinzha1@microsoft.com>

TerryHowe force-pushed the get-successors branch from 788d416 to 76e2615 Compare August 9, 2024 01:17

Increase test coverage for handlers

ecd9e7f

Signed-off-by: Terry Howe <terrylhowe@gmail.com>

TerryHowe force-pushed the get-successors branch from 76e2615 to ecd9e7f Compare August 9, 2024 12:38

qweeah added 2 commits August 9, 2024 15:41

test(unit): increase coverage

ec11e03

Signed-off-by: Billy Zha <jinzha1@microsoft.com>

Merge branch 'main' into get-successors

d9e22e8

TerryHowe merged commit 141c5e1 into oras-project:main Aug 9, 2024

qweeah mentioned this pull request Dec 9, 2024

bump: tag and release ORAS CLI v1.3.0-beta.1 #1577

Merged

Wwwsylvia mentioned this pull request Sep 3, 2025

bump: tag and release ORAS CLI v1.3.0 #1839

Merged

6 tasks

Conversation

TerryHowe commented Jul 11, 2024

Uh oh!

codecov Bot commented Jul 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

TerryHowe left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

qweeah commented Jul 17, 2024

Uh oh!

Uh oh!

TerryHowe left a comment

Choose a reason for hiding this comment

Uh oh!

shizhMSFT left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

TerryHowe commented Jul 22, 2024

Uh oh!

TerryHowe commented Jul 22, 2024

Uh oh!

qweeah left a comment

Choose a reason for hiding this comment

Uh oh!

qweeah commented Aug 6, 2024

Uh oh!

qweeah commented Aug 9, 2024

Uh oh!

TerryHowe commented Aug 9, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov Bot commented Jul 11, 2024 •

edited

Loading

shizhMSFT left a comment •

edited

Loading