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

Run basic hwinfo sanity test on build service #100

Merged
merged 7 commits into from
Dec 15, 2021

Conversation

djoreilly
Copy link
Collaborator

A test flag is added so the build service will run this test on
the x86, ARM and s390 builds. The flag is off by default so it does
not run on developers machines.

@djoreilly
Copy link
Collaborator Author

From the x86 build log

[  109s] === RUN   TestGetHwinfo
[  109s]     hwinfo_test.go:134: getHwinfo() failed: ExecuteError: Cmd: [dmidecode -s system-uuid], RC: -1, Error: exec: "dmidecode": executable file not found in $PATH, Output: 
[  109s] --- FAIL: TestGetHwinfo (0.02s)

Dmidecode is in the build requires. Maybe /usr/sbin is not in $PATH

@skazi0
Copy link
Contributor

skazi0 commented Dec 6, 2021

Dmidecode is in the build requires. Maybe /usr/sbin is not in $PATH

I see it only in Requires not in BuildRequires. I'll try to add it there but I first want to add the error handling as in the original.

@djoreilly
Copy link
Collaborator Author

Dmidecode is in the build requires. Maybe /usr/sbin is not in $PATH

I see it only in Requires not in BuildRequires. I'll try to add it there but I first want to add the error handling as in the original.

ah okay. Sounds like a good plan. Thanks

@skazi0 skazi0 force-pushed the run-hwinfo-test-on-BS branch 11 times, most recently from a4a669b to 32b6971 Compare December 7, 2021 15:05
@@ -121,7 +130,7 @@ find %_builddir/..
rm -rf %buildroot/usr/share/go

%check
%gotest -v %import_path/internal/connect
%gotest -v %import_path/internal/connect -test-hwinfo
Copy link
Collaborator

Choose a reason for hiding this comment

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

We were talking about running hwinfo here failing on OBS because it is running as a user, but accessing /dev/mem needs root.

There is a way to give builds access to root, but that needs whitelisting by an obs admin, so we should not use it:
https://github.com/openSUSE/open-build-service/blob/af9297d8d8fb79e14951f657a792474c53179e67/src/backend/BSConfig.pm.template#L220

So, no good way to do tests during rpm build that need root. So if that is the only way then it needs to happen in some other CI, like openqa. The openqa tests are currently run against MicroOS on s390, aarch64 and x86_64.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ok, so maybe the build service should not run with-test-hwinfo and instead do this test as root on openqa instead. Or maybe there is some env var that can be checked to determine if the tests are running openqa, or maybe we could just skip the hwinfo test if os.Geteuid() != 0

Copy link
Contributor

Choose a reason for hiding this comment

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

if we add this check, the test will most probably not be run anywhere.
I have another idea: add some macro "flag" in the spec file and make those tests conditional on that. This way we can branch the package, set this flag and get at least the non-root parts executed if needed. These tests have proven useful already and detected missing PPC implementation so I'd rather keep them around at least for manual run.

@skazi0 skazi0 force-pushed the run-hwinfo-test-on-BS branch 8 times, most recently from fb8dd73 to f0e59c6 Compare December 8, 2021 13:42
djoreilly and others added 5 commits December 8, 2021 15:07
A test flag is added so the build service will run this test on
the x86, ARM and s390 builds. The flag is off by default so it does
not run on developers machines.
suseconnect-ng.spec Show resolved Hide resolved
suseconnect-ng.spec Show resolved Hide resolved
internal/connect/hwinfo.go Outdated Show resolved Hide resolved
suseconnect-ng.spec Show resolved Hide resolved
internal/connect/hwinfo.go Outdated Show resolved Hide resolved
internal/connect/hwinfo.go Outdated Show resolved Hide resolved
@@ -121,7 +130,7 @@ find %_builddir/..
rm -rf %buildroot/usr/share/go

%check
%gotest -v %import_path/internal/connect
%gotest -v %import_path/internal/connect -test-hwinfo
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ok, so maybe the build service should not run with-test-hwinfo and instead do this test as root on openqa instead. Or maybe there is some env var that can be checked to determine if the tests are running openqa, or maybe we could just skip the hwinfo test if os.Geteuid() != 0

@skazi0 skazi0 force-pushed the run-hwinfo-test-on-BS branch 2 times, most recently from 6eb8166 to 75b0dd1 Compare December 14, 2021 21:52
@skazi0 skazi0 force-pushed the run-hwinfo-test-on-BS branch 4 times, most recently from 76aca4a to 3240dbb Compare December 15, 2021 09:41
Copy link
Collaborator Author

@djoreilly djoreilly left a comment

Choose a reason for hiding this comment

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

LGTM - but I'm not allowed to approve because I started the PR.

@skazi0
Copy link
Contributor

skazi0 commented Dec 15, 2021

@JanZerebecki can you take a quick look at the spec file changes?

@JanZerebecki
Copy link
Collaborator

I don't have time to look at it in detail, you will need to proceed without me. There are people outside of our team who have knowledge of openqa who you can contact, e.g. Jose username jlausuch on Github.

@skazi0
Copy link
Contributor

skazi0 commented Dec 15, 2021

@JanZerebecki thx!

@skazi0 skazi0 merged commit 171ae76 into SUSE:main Dec 15, 2021
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.

None yet

3 participants