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.

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

osbuild: include context in skopeo error messages #289

Merged
merged 2 commits into from
Dec 4, 2023

Conversation

mvo5
Copy link
Contributor

@mvo5 mvo5 commented Nov 29, 2023

When trying to create enough data for a full pipeline run for #287 I struggled a bit because it was not clear which items where faulty.

This commit adds more context to the errors. Sadly I had to implement assertPanicsWithErrorRegexp because of stretchr/testify#1304

I can contribute it upstream but for that it needs some tests around it first :)

@mvo5 mvo5 force-pushed the skopedo-more-err-context branch 2 times, most recently from 8adffb5 to b8cf781 Compare November 29, 2023 16:29
pkg/osbuild/skopeo_source_test.go Outdated Show resolved Hide resolved
pkg/osbuild/skopeo_source_test.go Show resolved Hide resolved
@achilleas-k achilleas-k changed the title osbuild: include context in skopedo error messages osbuild: include context in skopeo error messages Dec 1, 2023
@achilleas-k
Copy link
Member

Fixed a typo in the PR name. Hope you don't mind :)

This adds a new internal package with `testify/assert` extensions.
It contains the new 'PanicsWithErrorRegexp()` helper that can
be removed once upstream stretchr/testify#1304
is fixed (or we contributed this back).

It could not be added to `internal/test` as it would have created
cyclic dependencies when importing from `pkg/osbuild`.
When trying to create enough data for a full pipeline run for
osbuild#287 I struggled a bit
because it was not clear which items where faulty.

This commit adds more context to the errors. It uses the new
`assertx.PanicsWithErrorRegexp` that is needed because of
stretchr/testify#1304
(until fixed or contributed back).
Copy link
Contributor

@bcl bcl left a comment

Choose a reason for hiding this comment

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

Looks good!

@bcl bcl added this pull request to the merge queue Dec 4, 2023
Merged via the queue into osbuild:main with commit 5a07b27 Dec 4, 2023
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants