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

Add fwupdmgr attachment job (New) #1089

Merged
merged 7 commits into from
Apr 11, 2024

Conversation

stanley31huang
Copy link
Collaborator

Description

This is a follow up PR for the PR965, I have applied the workaround (provided by Pierre). And I have verified the tests on EGW 3200.
https://certification.canonical.com/hardware/202312-33237/submission/360846/

This attachment job is requested from the OEM SWE team. They'd like to parse the fwupdmgr device information from the submission.
This PR added a firmware/fwupdmgr_get_devices attachment job and the related test plan to be nested in the client-cert-desktop test plan.

The firmware/fwupdmgr_get_devices only does the following command:

fwupdmgr get-devices --json

Requires

requires:
    executable.name == "fwupdmgr"
    environment.SNAP == ""  # only execute on Debian checkbox

The tests will only be executed when fwupdmgr executable is available.
Also, we found a limitation:

Our purpose is to run the fwupdmgr command from the default deb package to get the needed information. However, while Checkbox is running in the snap environment, the fwupdmgr will try to find the snap services snap.fwupd.fwupd.service, which leads to the following daemon and client mismatch error,

root@u-Inspiron-15-5508:/home/u# fwupdmgr get-devices
Mismatched daemon and client, use fwupd.fwupdmgr instead

This environment check is written in https://github.com/fwupd/fwupd/blob/main/src/fu-util-common.c#L42

#define SYSTEMD_FWUPD_UNIT	"fwupd.service"
#define SYSTEMD_SNAP_FWUPD_UNIT "snap.fwupd.fwupd.service"

fu_util_get_systemd_unit(void)
{
	if (g_getenv("SNAP") != NULL)
		return SYSTEMD_SNAP_FWUPD_UNIT;
	return SYSTEMD_FWUPD_UNIT;
}

Using the Debian version checkbox will not encounter this problem. Currently, I don’t have any good idea to solve this problem. So I intend to add a job for Debian Checkbox for now. And the environment.SNAP == "" is to ensure the job will only be executed with Debian Checkbox.

Resolved issues

Request from SWE team
https://warthogs.atlassian.net/browse/OEMQA-3797

Tests

Sideload result on an Ubuntu Desktop 22.04 laptop with Debian Checkbox
https://pastebin.canonical.com/p/4Jf5gXDKYV/
Sideload result on an Ubuntu Desktop 22.04 laptop with Snap Checkbox -> job skipped
https://pastebin.canonical.com/p/8wnCchBnWv/

Copy link

codecov bot commented Mar 20, 2024

Codecov Report

Attention: Patch coverage is 94.44444% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 42.87%. Comparing base (8b61956) to head (5c875ed).
Report is 24 commits behind head on main.

❗ Current head 5c875ed differs from pull request most recent head 34588f1. Consider uploading reports for the commit 34588f1 to get more accurate results

Files Patch % Lines
providers/base/bin/get_firmware_info_fwupd.py 94.44% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1089      +/-   ##
==========================================
+ Coverage   42.85%   42.87%   +0.02%     
==========================================
  Files         351      352       +1     
  Lines       38443    38461      +18     
  Branches     6532     6536       +4     
==========================================
+ Hits        16475    16492      +17     
  Misses      21302    21302              
- Partials      666      667       +1     
Flag Coverage Δ
provider-base 15.36% <94.44%> (+0.09%) ⬆️
provider-certification-client 57.14% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@stanley31huang stanley31huang changed the title Add fwupdmgr log Add fwupdmgr attachment job (New) Mar 20, 2024
@stanley31huang stanley31huang changed the title Add fwupdmgr attachment job (New) Add fwupdmgr attachment job (New) Mar 20, 2024
Copy link
Contributor

@kissiel kissiel left a comment

Choose a reason for hiding this comment

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

Minor addition proposed in line, and one possible simplification.

providers/base/units/firmware/jobs.pxu Outdated Show resolved Hide resolved
providers/base/units/firmware/jobs.pxu Show resolved Hide resolved
@stanley31huang stanley31huang marked this pull request as draft March 29, 2024 03:33
@stanley31huang stanley31huang marked this pull request as ready for review April 1, 2024 02:03
@stanley31huang stanley31huang requested a review from a team as a code owner April 1, 2024 02:03
patliuu and others added 6 commits April 1, 2024 10:14
modified the workaround
removed the step to restore the SNAP env
implement a script to be able to retrieve frimware information by debian
fwupd and snap fwupd
Copy link
Contributor

@kissiel kissiel left a comment

Choose a reason for hiding this comment

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

The tests provided here look like they are here just to have test coverage instead of testing the logic (and the most difficult piece of logic is untested).

providers/base/bin/get_firmware_info_fwupd.py Outdated Show resolved Hide resolved
providers/base/bin/get_firmware_info_fwupd.py Outdated Show resolved Hide resolved
restructure scripts and added unittest cases
Copy link
Contributor

@kissiel kissiel left a comment

Choose a reason for hiding this comment

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

Awesome! Thanks for all the amendments!
+1

@kissiel kissiel merged commit 6e64f92 into canonical:main Apr 11, 2024
16 checks passed
LiaoU3 pushed a commit to LiaoU3/checkbox that referenced this pull request Apr 17, 2024
* Add fwupdmgr test job and plans

* Add it to client-cert-desktop test plan

* modified the workaround

modified the workaround

* Update providers/base/units/firmware/jobs.pxu

Co-authored-by: kissiel <[email protected]>

* removed the step to restore SNAP environ var

removed the step to restore the SNAP env

* retrieve firmware by debian and snap fwupd

implement a script to be able to retrieve frimware information by debian
fwupd and snap fwupd

* Restructure scripts and added unittest cases

restructure scripts and added unittest cases

---------

Co-authored-by: Patrick Liu <[email protected]>
Co-authored-by: kissiel <[email protected]>
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.

4 participants