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

Sanitize signing operation output #3424

Merged
merged 2 commits into from
Nov 6, 2024

Conversation

pmatilai
Copy link
Member

@pmatilai pmatilai commented Nov 4, 2024

Only output filenames on addsign/delsign success on verbose mode, no news is good news. Previusly we emitted non-sensical messages like this:

/some/path/my-1.rpm:
/some/path/my-2.rpm:

...presumably in anticipation of error messages, but it's saner to have error messages carry the path where relevant.

Loosely related to the multiple signatures stuff - this was driving me crazy when writing tests.

@pmatilai pmatilai requested a review from a team as a code owner November 4, 2024 11:53
@pmatilai pmatilai requested review from dmnks and removed request for a team November 4, 2024 11:53
Copy link
Contributor

@dmnks dmnks 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, I've made a little fixup commit, see the message for details. There's a conflict in one test on master now so please resolve and then feel free to merge this one.

In the fakechroot days there was just no way to get gpg to run
inside that contraption, with the "new" container stuff, we can just
run everything there. Makes life simpler in many ways.

GPG still needs GPG_TTY set, do this centrally from snapshot()
to get it out of all the individual tests.
--addsign and --delsign used to emit output like below, which makes
you expect some additional output, where there will be none:

	/some/path.rpm:

Make sure all the error messages give a path you can relate to,
only emit the file name in INFO level in case of success.
No news is good news, right? So on success you now only get the
following IFF operation succeeds and -v was specified:

	/some/path.rpm

This also means we don't need to filter out these meaningless messages
all over the place in the test-suite, remove now unndecessary /dev/null
redirects and add -v to one case of both --addsign and --delsign to
cover that case.
@pmatilai
Copy link
Member Author

pmatilai commented Nov 6, 2024

Hmm, I found one leftover /dev/null redirect in my local tree but not the one you mentioned, I wonder if this is GH playing tricks... anyway, fixed the one I found 😅

@dmnks
Copy link
Contributor

dmnks commented Nov 6, 2024

Hmm, I found one leftover /dev/null redirect in my local tree but not the one you mentioned, I wonder if this is GH playing tricks... anyway, fixed the one I found 😅

Yeah, I realized I had commented on an outdated commit so deleted the comment right away 😅 Juggling too many branches/PRs, indeed...

@dmnks dmnks merged commit 117e6b5 into rpm-software-management:master Nov 6, 2024
1 check 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.

2 participants