Skip to content
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

Don’t silently ignore errors when decoding JSON or deleting incomplete layers #1406

Merged
merged 3 commits into from
Oct 26, 2022
Merged

Don’t silently ignore errors when decoding JSON or deleting incomplete layers #1406

merged 3 commits into from
Oct 26, 2022

Conversation

mtrmac
Copy link
Collaborator

@mtrmac mtrmac commented Oct 21, 2022

See individual commit messages for details

Cc: @nalind to make sure I’m not missing some error resiliency trade-off.

I’ll run this through Buildah and Podman CI together with some follow-up PRs.

@mtrmac mtrmac mentioned this pull request Oct 21, 2022
Merged
@mtrmac
Copy link
Collaborator Author

mtrmac commented Oct 21, 2022 •
edited

Downstream tests (from #1407, a superset):

@mtrmac mtrmac marked this pull request as ready for review October 21, 2022 22:38
@mtrmac mtrmac marked this pull request as draft October 21, 2022 22:38
vrothberg
vrothberg reviewed Oct 25, 2022
View reviewed changes
Copy link
Member

@vrothberg vrothberg left a comment

Choose a reason for hiding this comment

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

LGTM

Let's wait for @nalind's head nod before merging.

@mtrmac
Don't silently ignore failures to decode metadata JSON
7c5820a 
Also tighten the scope of some error variables a bit.

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
@mtrmac
Don't silently ignore failures to delete incomplete layers
68ec0a1 
Signed-off-by: Miloslav Trmač <mitr@redhat.com>
@mtrmac
Remove always-true err == nil checks
f6776bf 
Signed-off-by: Miloslav Trmač <mitr@redhat.com>
@mtrmac
Copy link
Collaborator Author

mtrmac commented Oct 25, 2022

Rebased, and added one more commit to avoid more err == nil checks.

@mtrmac mtrmac marked this pull request as ready for review October 25, 2022 19:34
@nalind
Copy link
Member

nalind commented Oct 26, 2022

I still do a double-take when I see %#v being used as a format specifier instead of %q, but LGTM.

@mtrmac
Copy link
Collaborator Author

mtrmac commented Oct 26, 2022

I don’t personally have any preference in %q vs %#v, do you want me to change that to be consistent for layer IDs, one way or another?

@nalind
Copy link
Member

nalind commented Oct 26, 2022

Nah, it's already in too many places to be worth the churn.

@rhatdan rhatdan merged commit 46f384d into containers:main Oct 26, 2022
@mtrmac mtrmac deleted the errors branch October 27, 2022 07:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vrothberg vrothberg vrothberg left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@mtrmac @nalind @vrothberg @rhatdan