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

pkg/imagefilter/formatter: add shell output #1166

Merged
merged 1 commit into from
Jan 27, 2025

Conversation

schuellerf
Copy link
Contributor

This should implement the initial idea of a
copy'n'paste ready list of possibilities.

@schuellerf schuellerf requested a review from a team as a code owner January 24, 2025 16:08
@schuellerf schuellerf force-pushed the add-shell-output branch 2 times, most recently from 681c2b3 to b3db281 Compare January 24, 2025 16:10
Copy link
Contributor

@mvo5 mvo5 left a comment

Choose a reason for hiding this comment

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

This is very nice, thank you!

I would love a test as well, maybe something like

index c866d2a30..40640261a 100644
--- a/pkg/imagefilter/formatter_test.go
+++ b/pkg/imagefilter/formatter_test.go
@@ -65,6 +65,11 @@ func TestResultsFormatter(t *testing.T) {
                        },
                        `[{"distro":{"name":"test-distro-1"},"arch":{"name":"test_arch3"},"image_type":{"name":"qcow2"}},{"distro":{"name":"test-distro-1"},"arch":{"name":"test_arch"},"image_type":{"name":"test_type"}}]` + "\n",
                },
+               {
+                       "shell",
+                       []string{"test-distro-1:qcow2:test_arch3"},
+                       "test-distro-1 --type type:qcow2 --arch arch:test_arch3\n",
+               },
        } {
                res := make([]imagefilter.Result, len(tc.fakeResults))
                for i, resultSpec := range tc.fakeResults {

(I did not test this) is already enough?

@schuellerf
Copy link
Contributor Author

of course I missed the test 😭 sorry

@schuellerf schuellerf requested a review from mvo5 January 24, 2025 16:22
This should implement the initial idea of a
copy'n'paste ready list of possibilities.
Comment on lines +73 to +77
if len(errs) > 0 {
return errors.Join(errs...)
}

return nil
Copy link
Member

@supakeen supakeen Jan 24, 2025

Choose a reason for hiding this comment

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

I believe you can do return errors.Join(errs...) directly here. errors.Join discards nil-values in errors and returns nil when it only has nils in its arguments (or when called with an empty slice).

Copy link
Contributor

@mvo5 mvo5 left a comment

Choose a reason for hiding this comment

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

Thank you for adding the test. This looks fine to me now.

@supakeen supakeen added this pull request to the merge queue Jan 27, 2025
Merged via the queue into osbuild:main with commit e57e2eb Jan 27, 2025
18 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