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

Skip systemd dependent unit test when systemd is not available #1369

Merged
merged 1 commit into from
Sep 6, 2023

Conversation

zimnx
Copy link
Collaborator

@zimnx zimnx commented Sep 5, 2023

when unit test is run inside a container where systemd is not
available the test is failing. It works fine when test binary is ran directly on the host,
and with NodeConfig because NC DaemonSet chroots into host where systemd is present.

@zimnx zimnx added the kind/failing-test Categorizes issue or PR as related to a consistently or frequently failing test. label Sep 5, 2023
@scylla-operator-bot scylla-operator-bot bot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Sep 5, 2023
Copy link
Member

@tnozicka tnozicka left a comment

Choose a reason for hiding this comment

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

/hold

I don't know, this feels out of place. Conditional unit tests not executed in the CI seem weird and probably the place where it breaks at some point.

If a unit test needs system wide installation of another component it may not be a good unit test.

AFAIK, systemd runs fine inside containers we just don't start it, neither I am sure whether we should but it's an option.

@scylla-operator-bot scylla-operator-bot bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 5, 2023
@tnozicka
Copy link
Member

tnozicka commented Sep 6, 2023

hm, I don't see a quick way out of it and we need to get our unit suite working. I've filled #1379 but let's not take the conditional unit as a precedence.

Please reference the issue in the code change.

/hold cancel

@scylla-operator-bot scylla-operator-bot bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 6, 2023
@tnozicka tnozicka added the priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. label Sep 6, 2023
when unit test is run inside a container where systemd is not
available the test is failing. It works fine when test binary is ran
directly on the host,
and with NodeConfig because NC DaemonSet chroots into host where systemd
is present.
@zimnx
Copy link
Collaborator Author

zimnx commented Sep 6, 2023

Please reference the issue in the code change.

Added reference.

Copy link
Member

@tnozicka tnozicka left a comment

Choose a reason for hiding this comment

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

/approve
/lgtm

thanks

@scylla-operator-bot scylla-operator-bot bot added the lgtm Indicates that a PR is ready to be merged. label Sep 6, 2023
@scylla-operator-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: tnozicka, zimnx

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@scylla-operator-bot scylla-operator-bot bot merged commit 3901f5a into scylladb:master Sep 6, 2023
11 checks passed
@zimnx zimnx deleted the mz/skip-systemd-test branch September 6, 2023 13:01
zimnx added a commit to zimnx/scylla-operator that referenced this pull request Sep 7, 2023
Test started failing due to bug in scylladb#1369. This time it passes on
systemd-enabled systems, and within container where it's not available.
@zimnx zimnx mentioned this pull request Sep 7, 2023
rzetelskik pushed a commit to rzetelskik/scylla-operator that referenced this pull request Sep 8, 2023
Test started failing due to bug in scylladb#1369. This time it passes on
systemd-enabled systems, and within container where it's not available.
rzetelskik pushed a commit to rzetelskik/scylla-operator that referenced this pull request Sep 11, 2023
Test started failing due to bug in scylladb#1369. This time it passes on
systemd-enabled systems, and within container where it's not available.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. kind/failing-test Categorizes issue or PR as related to a consistently or frequently failing test. lgtm Indicates that a PR is ready to be merged. priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants