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 simple smoke test building drbd using the kernel-module-container #316

Merged
merged 3 commits into from
Jan 2, 2024

Conversation

dcermak
Copy link
Collaborator

@dcermak dcermak commented Oct 6, 2023

No description provided.

@dcermak dcermak force-pushed the kernel-module-container-tests branch from 012bfc0 to 61c1feb Compare October 6, 2023 06:30
@brunoleon
Copy link

I think you can mark it ready for review, this looks fine.

pyproject.toml Outdated Show resolved Hide resolved
tests/test_all.py Outdated Show resolved Hide resolved
tests/test_all.py Outdated Show resolved Hide resolved
@dcermak dcermak force-pushed the kernel-module-container-tests branch 2 times, most recently from 42511a8 to 207b092 Compare October 10, 2023 10:00
@dcermak dcermak requested a review from dirkmueller October 10, 2023 10:01
@dcermak dcermak force-pushed the kernel-module-container-tests branch 2 times, most recently from f778994 to 97ee60f Compare October 12, 2023 10:00
Copy link
Member

@dirkmueller dirkmueller left a comment

Choose a reason for hiding this comment

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

generally lgtm, thanks a lot. just don't want to add any further exceptions to the BCI repo test..

tests/test_all.py Outdated Show resolved Hide resolved
@dcermak dcermak force-pushed the kernel-module-container-tests branch from 97ee60f to a854904 Compare October 12, 2023 14:24
@dcermak dcermak force-pushed the kernel-module-container-tests branch from a854904 to e4335ad Compare October 20, 2023 08:19
@dcermak dcermak marked this pull request as ready for review October 20, 2023 08:19
@dcermak dcermak force-pushed the kernel-module-container-tests branch 3 times, most recently from 1e71f60 to 48e95bf Compare October 25, 2023 12:44
dirkmueller
dirkmueller previously approved these changes Oct 29, 2023
@dcermak dcermak force-pushed the kernel-module-container-tests branch from 48e95bf to 63be77b Compare November 1, 2023 16:12
@dcermak dcermak force-pushed the kernel-module-container-tests branch from 63be77b to 06b5a1d Compare November 1, 2023 16:14
@dcermak dcermak force-pushed the kernel-module-container-tests branch from 06b5a1d to 362bcb5 Compare December 13, 2023 15:22
@dcermak dcermak force-pushed the kernel-module-container-tests branch from 362bcb5 to cb6896d Compare December 21, 2023 15:21
@dirkmueller dirkmueller force-pushed the kernel-module-container-tests branch 3 times, most recently from 09eb0e8 to c7f148a Compare January 2, 2024 10:00
dirkmueller
dirkmueller previously approved these changes Jan 2, 2024
@dirkmueller dirkmueller force-pushed the kernel-module-container-tests branch from c7f148a to d487481 Compare January 2, 2024 10:10
Copy link

@ricardobranco777 ricardobranco777 left a comment

Choose a reason for hiding this comment

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

Small suggestion, otherwise LGTM.

Comment on lines +295 to +307
if (
c
not in (
[
INIT_CONTAINER,
PCP_CONTAINER,
# kernel-module-container contains systemd due to pesign,
# fixes are pending
KERNEL_MODULE_CONTAINER,
]
+ POSTGRESQL_CONTAINERS
)
)
Copy link

@ricardobranco777 ricardobranco777 Jan 2, 2024

Choose a reason for hiding this comment

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

I would use a set here like this:

Suggested change
if (
c
not in (
[
INIT_CONTAINER,
PCP_CONTAINER,
# kernel-module-container contains systemd due to pesign,
# fixes are pending
KERNEL_MODULE_CONTAINER,
]
+ POSTGRESQL_CONTAINERS
)
)
if c not in {INIT_CONTAINERS, PCP_CONTAINER, KERNEL_MODULE_CONTAINER} | {POSTGRESQL_CONTAINERS}

Copy link
Member

Choose a reason for hiding this comment

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

@ricardobranco777 this isn't working (and there is a typo INIT_CONTAINERS->INIT_CONTAINER) because we're mixing sets and lists here (e.g. postgresql_containers is a list, so {[1,2,3]} gives

>>> {[1,2,3]}
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: unhashable type: 'list'

Choose a reason for hiding this comment

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

@ricardobranco777 this isn't working (and there is a typo INIT_CONTAINERS->INIT_CONTAINER) because we're mixing sets and lists here (e.g. postgresql_containers is a list, so {[1,2,3]} gives

>>> {[1,2,3]}
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: unhashable type: 'list'

You can use set(POSTGRESQL_CONTAINERS).

Copy link
Member

Choose a reason for hiding this comment

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

@ricardobranco777 same issue:

tests/test_all.py:298: in <listcomp>
    {
<string>:3: in __hash__
    ???
E   TypeError: unhashable type: 'list'

I don't have time to debug this right now. lets take this into a followup PR.

@dirkmueller dirkmueller force-pushed the kernel-module-container-tests branch from 67371fa to 81c560c Compare January 2, 2024 13:36
Copy link
Member

@mgrossu mgrossu left a comment

Choose a reason for hiding this comment

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

LGTM

@dirkmueller dirkmueller merged commit 43fba17 into main Jan 2, 2024
137 of 138 checks passed
@dirkmueller dirkmueller deleted the kernel-module-container-tests branch January 2, 2024 15:25
rcmadhankumar pushed a commit that referenced this pull request Aug 30, 2024
Add simple smoke test building drbd using the kernel-module-container
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.

5 participants