User/gmileka/test branch - #18696
Conversation
|
Hello, and thank you for opening this pull request! 👋🏼 We appreciate the contribution. We require pull request titles to follow the Conventional Commits specification and it looks like your proposed title needs to be adjusted. Commits in this repo will typically be prefixed with Details: |
There was a problem hiding this comment.
🟡 Changes recommended
The test package is not registered as a component, signing can produce false positives, and generated specs remain stale.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds xsign-proxy signing checks to fwupd-related builds and introduces a test package.
Changes:
- Adds direct
pesign-clientsigning paths. - Adds an xsign-proxy test RPM.
- Refreshes component locks and rendered specs.
File summaries
| File | Description |
|---|---|
base/comps/components.toml |
Moves fwupd to dedicated configuration. |
base/comps/fwupd/fwupd.comp.toml |
Adds a signing-test overlay. |
base/comps/fwupd-efi/fwupd-efi.comp.toml |
Adds certificate-based EFI signing. |
locks/fwupd.lock |
Refreshes fwupd fingerprint. |
locks/fwupd-efi.lock |
Refreshes fwupd-efi fingerprint. |
specs/f/fwupd/fwupd.spec |
Contains rendered fwupd changes. |
specs/f/fwupd-efi/fwupd-efi.spec |
Contains rendered EFI signing changes. |
specs/x/xsign-proxy-test/xsign-proxy-test.spec |
Adds the test RPM specification. |
specs/x/xsign-proxy-test/README |
Documents the test package. |
Review details
Suppressed comments (1)
specs/x/xsign-proxy-test/xsign-proxy-test.spec:87
- This repeats the unsigned input path, making
[ -f "$SIGNED_TEST_FILE" ]true even when no signing branch ran and causing the package to install the unsigned fixture as if it were signed. In the canonical local spec, point this variable at the distinct signed output created in%build.
SIGNED_TEST_FILE="%{_builddir}/%{name}-%{version}/test-file-%{name}-%{version}.txt"
- Files reviewed: 7/9 changed files
- Comments generated: 8
- Review effort level: Balanced
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| --sign \ | ||
| --certificate "cp-459159-pgp" \ | ||
| --token "unused-token-name" \ | ||
| --infile "$TEST_FILE" \ |
| @@ -0,0 +1,63 @@ | |||
| [components.fwupd] | |||
|
|
|||
| [[components.fwupd.overlays]] | |||
| %changelog | ||
| ## START: Generated by rpmautospec | ||
| * Wed Sep 02 2026 azldev <azldev@local> - 1.8-4 | ||
| - Local changes (uncommitted) |
| %changelog | ||
| ## START: Generated by rpmautospec | ||
| * Wed Sep 02 2026 azldev <azldev@local> - 2.0.19-3 | ||
| - Local changes (uncommitted) |
| @@ -0,0 +1,102 @@ | |||
| Name: xsign-proxy-test | |||
| # Are we running on a secure-boot image? | ||
| if [ -x "$PESIGN_CLIENT" ]; then | ||
|
|
||
| SIGNED_TEST_FILE="%{_builddir}/%{name}-%{version}/test-file-%{name}-%{version}.txt" |
| fi | ||
| echo "PASS: pesign sign request completed successfully" |
| @@ -0,0 +1,102 @@ | |||
| Name: xsign-proxy-test | |||
| Version: 1.0.0 | |||
| Release: 1%{?dist} | |||
There was a problem hiding this comment.
🟡 Changes recommended
Signing correctness, missing component registration, and stale rendered changelogs must be addressed.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (4)
Previously missed (3) — in code that hasn't changed since the last review.
specs/x/xsign-proxy-test/xsign-proxy-test.spec:3
- This adds the package only under
specs/, whichbase/project.toml:13declares as generated rendered output. No component definition or lock exists forxsign-proxy-test, so azldev cannot discover, update, render, or build it, and rendered-spec cleanup can discard it. Add a dedicated local component underbase/comps/xsign-proxy-test/, use the repository's%autorelease/%autochangelogconvention for new local specs, generate its lock, and re-render the output.
specs/x/xsign-proxy-test/xsign-proxy-test.spec:37 SIGNED_TEST_FILEis identical toTEST_FILE, so it exists before any signing request. As a result, the later existence checks and%installpath cannot distinguish a signed result from the original unsigned text, and an unset/unsupported signing type still reports PASS and packages the original file. Use a distinct.signedoutput consistently here and in%install, and fail when no requested signing mode ran.
specs/f/fwupd/fwupd.spec:430- The generated changelog still says
Local changes (uncommitted), so it was rendered before the component commit existed. Repository guidance in.github/copilot-instructions.md:56requires a post-commit render-and-amend; otherwise Check Rendered Specs will detect drift.
base/comps/fwupd/fwupd.comp.toml:33
TEST_FILEis never initialized anywhere in the fwupd spec, so whenever this branch is entered,pesign-client --infile "$TEST_FILE"receives an empty path and the laterls "$TEST_FILE"fails the build. This overlay appears to copy test-package logic into fwupd without creating an input artifact; remove it or define and create the intended artifact before signing.
--infile "$TEST_FILE" \
- Files reviewed: 7/9 changed files
- Comments generated: 2
- Review effort level: Balanced
| else | ||
| %pesign -s -i %{fwup_efi_fn} -o %{fwup_efi_fn}.signed | ||
| fi | ||
|
|
||
| ls -la "$TEST_FILE" | ||
| ls -la "$SIGNED_TEST_FILE"''' | ||
|
|
||
| [[components.fwupd-efi.overlays]] | ||
| description = "Remove cleanup for the eliminated intermediate signed EFI file" | ||
| type = "spec-search-replace" | ||
| section = "%install" | ||
| regex = '^rm -vf %\{fwup_efi_fn\}\.tmp$' |
| %changelog | ||
| ## START: Generated by rpmautospec | ||
| * Wed Sep 02 2026 azldev <azldev@local> - 1.8-5 | ||
| - Local changes (uncommitted) |
There was a problem hiding this comment.
🔵 Needs a closer look
The test component is unregistered, can report success without signing, and the fwupd-efi rendered output is stale.
Review details
Suppressed comments (6)
Previously missed (2) — in code that hasn't changed since the last review.
specs/f/fwupd-efi/fwupd-efi.spec:132
gwupdappears to be a typo forfwupd; nogwupdcomponent or other reference exists in the repository. Correct the source commit message and re-render rather than editing this generated changelog directly.
specs/x/xsign-proxy-test/xsign-proxy-test.spec:43- The option name is misspelled in both examples;
pesign-clientuses--certificate.
specs/x/xsign-proxy-test/xsign-proxy-test.spec:1
- This file is being added only under
specs/, but that tree is generated output (.github/copilot-instructions.md:28-43) and there is no[components.xsign-proxy-test]definition or lock in the repository. As a result, azldev cannot render/build this package, and a render-all with--clean-stalewill prune the directory. Add it as a local component underbase/comps/xsign-proxy-test/, generate its lock, and then commit the rendered output.
Name: xsign-proxy-test
specs/x/xsign-proxy-test/xsign-proxy-test.spec:75
- An empty or unrecognized
SIGULDRY_PESIGN_BRIDGE_SIGNING_TYPESvalue skips everypesign-clientbranch, but execution still reportsPASS. That lets this communication test succeed without ever contacting xsign-proxy; fail explicitly when no supported signing type is selected.
echo "PASS: pesign sign request completed successfully"
specs/x/xsign-proxy-test/xsign-proxy-test.spec:3
- This newly authored local spec uses a static release and handwritten changelog instead of the repository's
%autorelease/%autochangelogconvention (seebase/comps/telegraf/telegraf.comp.toml:7-9andtelegraf.spec:24-25). Convert both macros and set the new component's release calculation toautoreleaseso azldev can keep release history synchronized.
Release: 1%{?dist}
specs/f/fwupd-efi/fwupd-efi.spec:129
- The generated changelog still contains
Local changes (uncommitted), proving this spec was rendered before the component commit existed. Per.github/copilot-instructions.md:54-60, re-render after committing and amend the result; otherwise the rendered-spec CI check will regenerate different release/changelog content.
* Wed Sep 02 2026 azldev <azldev@local> - 1.8-5
- Local changes (uncommitted)
- Files reviewed: 4/5 changed files
- Comments generated: 0 new
- Review effort level: Balanced
No description provided.