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

Switch to golang native error wrapping #1077

Merged
merged 1 commit into from
Jul 12, 2022
Merged

Switch to golang native error wrapping #1077

merged 1 commit into from
Jul 12, 2022

Conversation

saschagrunert
Copy link
Member

github.com/pkg/errors is deprecated since quite some time so we now use the native error wrapping for more idiomatic golang.

@saschagrunert saschagrunert changed the title Switch to golang native error wrapping WIP: Switch to golang native error wrapping Jun 28, 2022
Luap99
Luap99 reviewed Jun 28, 2022
View reviewed changes
libimage/image.go Outdated Show resolved Hide resolved
@rhatdan
Copy link
Member

rhatdan commented Jun 28, 2022

Could you open a WIP PR with this change against podman, to make sure that these changes do not break any tests.

@saschagrunert saschagrunert mentioned this pull request Jun 28, 2022
Closed
@saschagrunert
Copy link
Member Author

Could you open a WIP PR with this change against podman, to make sure that these changes do not break any tests.

Testing in containers/podman#14753

@saschagrunert saschagrunert changed the title WIP: Switch to golang native error wrapping Switch to golang native error wrapping Jun 28, 2022
@rhatdan
Copy link
Member

rhatdan commented Jul 7, 2022

LGTM
@containers/podman-maintainers PTAL
@vrothberg @giuseppe @flouthoc PTAL

@Luap99
Copy link
Member

Luap99 commented Jul 7, 2022

/hold
Do not merge until containers/podman#14784 is done.

@rhatdan
Copy link
Member

rhatdan commented Jul 7, 2022

It would be nice to have this all completed before we ship podman 4.2.

@vrothberg
Copy link
Member

It would be nice to have this all completed before we ship podman 4.2.

RC-1 is scheduled for next Monday. We can do the vendor dance after. Maybe we can get cosign/sigstore merged as well.

@rhatdan
Copy link
Member

rhatdan commented Jul 7, 2022

That would be awesome.

@rhatdan
Copy link
Member

rhatdan commented Jul 7, 2022

We should vendor in everything we can now, to make sure RC1 is close as possible to being correct.

@vrothberg
Copy link
Member

vrothberg commented Jul 7, 2022 •
edited
Loading

We should vendor in everything we can now, to make sure RC1 is close as possible to being correct.

I suggest to wait. The vendor dance consumes a lot of time and work and a c/image bump would require to effectively do it twice if started too early.

@saschagrunert saschagrunert mentioned this pull request Jul 8, 2022
Closed
@saschagrunert
Copy link
Member Author

Giving it a new test in containers/podman#14869

@rhatdan
Copy link
Member

rhatdan commented Jul 11, 2022

@saschagrunert Can you update this PR?

@saschagrunert
Copy link
Member Author

@saschagrunert Can you update this PR?

Done 👍

vrothberg
vrothberg reviewed Jul 12, 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
/hold

@openshift-ci openshift-ci bot added the lgtm label Jul 12, 2022
flouthoc
flouthoc approved these changes Jul 12, 2022
View reviewed changes
Copy link
Collaborator

@flouthoc flouthoc left a comment

Choose a reason for hiding this comment

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

LGTM
/lgtm
/hold cancel

This was referenced Jul 12, 2022
Closed
Merged
@saschagrunert
Switch to golang native error wrapping
e6f0784 
`github.com/pkg/errors` is deprecated since quite some time so we now
use the native error wrapping for more idiomatic golang.

Signed-off-by: Sascha Grunert <sgrunert@redhat.com>
@openshift-ci openshift-ci bot removed the lgtm label Jul 12, 2022
@mtrmac mtrmac mentioned this pull request Jul 12, 2022
Merged
@rhatdan
Copy link
Member

rhatdan commented Jul 12, 2022

/approve

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jul 12, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: flouthoc, rhatdan, saschagrunert

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

@rhatdan
Copy link
Member

rhatdan commented Jul 12, 2022

/lgtm

@openshift-ci openshift-ci bot added the lgtm label Jul 12, 2022
@openshift-ci openshift-ci bot merged commit 7979683 into containers:main Jul 12, 2022
@saschagrunert saschagrunert deleted the errors branch July 12, 2022 11:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Luap99 Luap99 Luap99 left review comments

@vrothberg vrothberg vrothberg left review comments

@flouthoc flouthoc flouthoc approved these changes

Assignees

@rhatdan rhatdan

@flouthoc flouthoc

@vrothberg vrothberg

Labels
approved lgtm
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@saschagrunert @rhatdan @Luap99 @vrothberg @flouthoc