fix: improve errors for ReadAll and loadIndexfile
#526
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
sparr
requested review from
sajayantony,
shizhMSFT,
SteveLasker and
Wwwsylvia
as code owners
Codecov Report
❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more. @@ Coverage Diff @@
## main #526 +/- ##
==========================================
- Coverage 73.22% 73.21% -0.02%
==========================================
Files 49 49
Lines 4602 4604 +2
==========================================
+ Hits 3370 3371 +1
- Misses 923 924 +1
Partials 309 309
|
shizhMSFT
reviewed
Jun 25, 2023
|
@sparr Could you address the comment and rebase the branch? We are ready to merge 🚀 |
shizhMSFT
changed the title
ReadAll and loadIndexfile
shizhMSFT
changed the title
ReadAll and loadIndexfileReadAll and loadIndexfile
sparr
force-pushed
the
432_loadIndexFile_error_clarity
branch
from
5ef63f4 to
a18e874
Compare
|
It looks like the branch is still out-of-date. @sparr Would you mind rebasing it? |
Signed-off-by: Clarence "Sparr" Risher <[email protected]>
sparr
force-pushed
the
432_loadIndexFile_error_clarity
branch
from
a18e874 to
bb97f3e
Compare
|
I'm also ok with contributors rebasing the PR. |
Wwwsylvia
pushed a commit
to Wwwsylvia/oras-go
that referenced
this pull request
Jul 4, 2023
NewWithContext refers to both the `oci-layout` and `index.json` as "OCI Image Layout", this PR fixes the latter. ReadAll is a deeper change that will improve error reporting for a lot of cases, at the cost of error volume. Some or all of that change could happen higher up to satisfy this particular issue. I think the expected/actual size output belongs here. There seem to be more than a few code paths in this and other repos that end up calling ReadAll multiple times without reporting which call produced an error so I think it's appropriate to also output the digest here, but that's less compelling. The Verify comment clears up confusion I encountered while working on this issue. "verifies size" implies checking for both too big AND too small, but it only checks if the file is bigger (or reader is longer) than expected. Fixes oras-project#432 Signed-off-by: Clarence "Sparr" Risher <[email protected]>
shizhMSFT
pushed a commit
that referenced
this pull request
Jul 4, 2023
NewWithContext refers to both the `oci-layout` and `index.json` as "OCI Image Layout", this PR fixes the latter. ReadAll is a deeper change that will improve error reporting for a lot of cases, at the cost of error volume. Some or all of that change could happen higher up to satisfy this particular issue. I think the expected/actual size output belongs here. There seem to be more than a few code paths in this and other repos that end up calling ReadAll multiple times without reporting which call produced an error so I think it's appropriate to also output the digest here, but that's less compelling. The Verify comment clears up confusion I encountered while working on this issue. "verifies size" implies checking for both too big AND too small, but it only checks if the file is bigger (or reader is longer) than expected. Fixes #432 Signed-off-by: Clarence "Sparr" Risher <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
NewWithContext refers to both the
oci-layoutandindex.jsonas "OCI Image Layout", this PR fixes the latter.ReadAll is a deeper change that will improve error reporting for a lot of cases, at the cost of error volume. Some or all of that change could happen higher up to satisfy this particular issue. I think the expected/actual size output belongs here. There seem to be more than a few code paths in this and other repos that end up calling ReadAll multiple times without reporting which call produced an error so I think it's appropriate to also output the digest here, but that's less compelling.
The Verify comment clears up confusion I encountered while working on this issue. "verifies size" implies checking for both too big AND too small, but it only checks if the file is bigger (or reader is longer) than expected.
Fixes #432