Skip to content

Conversation

cgwalters
Copy link
Collaborator

Followup to previous commit.

Followup to previous commit.

Signed-off-by: Colin Walters <walters@verbum.org>
@cgwalters cgwalters enabled auto-merge September 5, 2025 20:56
@bootc-bot bootc-bot bot requested a review from jeckersb September 5, 2025 20:56
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request adds a test to verify that a journal message is emitted when bootc switch is run, which is a good addition for ensuring logging behavior. My feedback focuses on improving the maintainability of the test by avoiding a magic string for the message ID.

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request adds a test to verify that a journal message is emitted during a bootc switch operation. However, the test is checking for the message ID of an in-place switch, while the test itself performs a standard switch. My review corrects this ID and suggests a more explicit way to verify the journal entry exists, improving the test's correctness and robustness.


# Also test that the mtime changes on modification
# Verify that we logged to the journal
journalctl _MESSAGE_ID=3e2f1a0b9c8d7e6f5a4b3c2d1e0f9a8b7
Copy link
Contributor

Choose a reason for hiding this comment

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

critical

The message ID being checked appears to be incorrect for this test case. The bootc switch command executed on line 63 does not include the --mutate-in-place flag, so it performs a standard switch operation. The journal message for a standard switch has the ID 7a6b5c4d3e2f1a0b9c8d7e6f5a4b3c2d1, not 3e2f1a0b9c8d7e6f5a4b3c2d1e0f9a8b7 which is for an in-place switch.

Additionally, the check is implicit, relying on the exit code of journalctl. It would be more robust and explicit to check that at least one log entry is found, for example by explicitly checking the exit code of the command.

let result = do --ignore-errors { journalctl --quiet _MESSAGE_ID=7a6b5c4d3e2f1a0b9c8d7e6f5a4b3c2d1 }
assert $result.exit_code == 0

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh wow, well spotted gemini except...actually the problem is switch in place and switch have the same message ID, which was probably (?) not intentional.

Copy link
Contributor

Choose a reason for hiding this comment

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

so do we change the ID then?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah I think we should change one of them, but I would say it should be a separate PR?

@cgwalters cgwalters merged commit 09b6c1c into bootc-dev:main Sep 10, 2025
34 of 35 checks passed
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.

2 participants