Add GNOME Initial Setup post-install test #517
No reviewers
Labels
No labels
Closed As
Duplicate
Closed As
Fixed
Closed As
Invalid
easyfix
freeipa
improvetest
iot
newtest
silverblue
tracker
ai-review-please
Backlog Status
Needs Review
Backlog Status
Ready
chore
documentation
points
01
points
02
points
03
points
05
points
08
points
13
Priority
Critical
Priority
High
Priority
Low
Priority
Medium
Sprint Status
Blocked
Sprint Status
Done
Sprint Status
In Progress
Sprint Status
Review
Sprint Status
To Do
Technical Debt
Work Item
Bug
Work Item
Epic
Work Item
Spike
Work Item
Task
Work Item
User Story
No milestone
No project
No assignees
4 participants
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference
quality/os-autoinst-distri-fedora!517
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "newtest/gnome-initial-setup"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Introduce a new GNOME Initial Setup test suite that resets the existing
user state, runs through the GIS wizard, and verifies that the selected
settings were applied correctly after first login.
The new test covers:
Add the required GIS needles, define a new gnome_initial_setup test
scenario in templates.fif.json
Fixes: #506
AI Code Review
📋 MR Summary
This MR introduces a new GNOME Initial Setup test suite that resets the existing user state, runs through the GIS wizard, and verifies that the selected settings were applied correctly after the first login.
check-needles.pyentries and multiple JSON needles for GNOME Initial Setup UI interactions.templates.fif.jsonentries forgnome_initial_setupandscreen_recordingtests, and alters QEMURAM.aaa_setup.pmto clear current user state and reboot the system to trigger the setup wizard.gnome_initial_setup.pmto drive the initial setup wizard interaction programmatically.post_scriptum.pmto assert that settings (locale, timezone, privacy, repos, user) were successfully applied.templates.fif.json, which could have downstream effects on performance or stability of other test suites relying on this configuration.Detailed Code Review
The MR introduces a comprehensive set of tests to cover the GNOME Initial Setup. The code is well-structured and broken down into preparation, interaction, and validation stages. However, modifying
QEMURAMintemplates.fif.jsonmight unintentionally impact other tests and its rationale is not explained in the commit message. There is also a missing input validation around the serial console setup, and hardcoding layout adjustments for typingloadkeys uscould be flaky if the previous layout state isn't perfectly predictable.📂 File Reviews
📄 `templates.fif.json` - Adds new test profiles and reduces QEMURAM.
Questions:
📄 `tests/applications/gnome-initial-setup/aaa_setup.pm` - Sets up the environment by deleting the current user and rebooting.
loginctl kill-user.📄 `tests/applications/gnome-initial-setup/post_scriptum.pm` - Validates system settings after the initial setup completes.
loadkezs usassumes the exact layout mapped 'y' to 'z'. If the keyboard layout is not exactly what is expected (e.g., QWERTY instead of QWERTZ), this command will fail to execute correctly.run_as_usercan lead to command injection if$useror$cmdcontains malicious characters. While it's a test environment, proper escaping should be used.$cmdand$userare properly sanitized or passed securely without shell evaluation risks.✅ Summary
loadkezs us) in post_scriptum.pm to prevent test flakiness.run_as_userwrapper to prevent command injection risks.userdel -rto avoid failures due to locked files.🤖 AI Code Review | Generated with ai-code-review | Model:
gemini-3.1-pro-preview⚠️ AI-generated suggestions may be incorrect. Verify before applying. Not a replacement for human review.
1bba2ea808d20947f173d20947f1733f88d72dacAI review:
loginctl kill-user- do not how serious that is when I am logging out correctly, but it cannot do any harm (fixed)run_as_user- nothing dangerous will be injected from a static script.3f88d72dac749b4e6737749b4e67372786dfc13c2786dfc13c9194e737d9We have
console_loadkeys_usinutil.pm. It's just a function for doing exactly what you did here (type the string that we know will produce the correct output for the specified...well, it usesLANGUAGE, should maybe change toLAYOUT, but meh). But it's centralized, so maybe add this language to that function and use it? I think conceptually this is fine, though: after all, if the layout is not the one we think it should be when we type this string, that's probably a bug so failing is fine.9194e737d984942abe18A few notes in line, but overall this looks nice, thanks!
@ -0,0 +11,4 @@my $username = get_var("USER_LOGIN") // "test";# Logout from the Gnome session to end up all user activites.send_key("super");this is fragile. We have
check_desktopto do this robustly; probably best use it instead.@ -0,0 +22,4 @@# Remove the user clear the system and force the gnome-initial-setup# to start on new boot.assert_script_run("loginctl kill-user $username");could we possibly use another loginctl command here to kill the user's session too, so we don't have to handle closing the overview and logging out interactively?
@adamwill wrote in #517 (comment):
The command already kills everything, so logging out was not necessary (I just wanted to do it super properly).
@ -0,0 +30,4 @@}sub test_flags {return {fatal => 1, milestone => 1};this does not need to be a milestone as we do not ever need to rollback in this test.
Logically there's no need for there to be three separate modules at all, right? This could just be one big one. If you like the separation just because one big module would be too long to read comfortably, that's fine I think, but maybe give the final module a more prosaic name,
gnome_initial_setup_post.pmor something.@ -0,0 +23,4 @@my $matched = check_screen([keys %tags], 10);for my $tag (@{$matched->{needle}->{tags}}) {return $tags{$tag} if exists $tags{$tag};hmm, neat.
@ -0,0 +44,4 @@sub handle_privacy {# Both options should be on by default.# If they are not, record a soft-fail and enable them.shouldn't we test whether disabling one of them works?
We now disable one of the services.
@ -0,0 +101,4 @@# Keep running through screens until we reach the final screenmy $iterations = 0;while (1) {die("GNOME Initial Setup appears stuck") if ++$iterations > 20;ah, this is quite neat, i like it.
@adamwill wrote in #517 (comment):
Not from my mind, unfortunately :D
@ -0,0 +106,4 @@my $current_screen = get_screen();record_info("GNOME Initial Setup", "Testing screen: $current_screen");last if $current_screen eq 'final';there is a problem here, though: if there's a bug such that a screen which should be shown is not, we won't catch it. imagine if every screen was broken - we'd still pass here.
maybe we should register all encountered screens and have a list of expected ones, and compare them when we exit this loop and fail if an expected screen wasn't encountered?
@adamwill wrote in #517 (comment):
I think we would not. If I fix the above problem and disable one of the options, then each screen will represent a change in the settings to the system. In that case, we'll fail in the follow-up tests because that change will not change anything.
Let me know, if my assumption is correct and we keep this, or you want me to add a pre-defined list of screens to compare with.
Ah, hmm, interesting. Yeah...if we take an action on each screen and validate it later...that should be theoretically sufficient. Although it makes the failure case a bit indirect - we'll fail in the verification stage and have to work backwards to "oh, verification failed because the screen wasn't there at all". Still, that might be OK.
There's also, I guess, a theoretical risk of a 'double bug', where a page isn't displayed and a default value on it is not as expected, but actually matches the value we try to 'change' to. In that case we would false-pass. That's probably very unlikely, though.
@ -0,0 +135,4 @@# When we have reached the final screenassert_and_click("gis_button_start_fedora");# When we have reached the system, dismiss the tourwe have
handle_welcome_screenfor this, though maybe it's a bit overkill?@ -0,0 +140,4 @@send_key("ret");wait_still_screen(3);# System starts with Activities mode, end itsend_key("esc");why bother? we don't do anything here after this anyway.
@ -0,0 +72,4 @@# FIXME: Workaround, currently we have a bug# https://bugzilla.redhat.com/show_bug.cgi?id=2446745 and third party repos are not# enabled, even if clicked in GIS. This will record a soft fail and enable them for now.# When the bug is gone, we should delete the workaround.the fix for this has landed, I think we can remove the workaround?
@ -0,0 +82,4 @@else {record_info("Workaround for BZ2446745", "It seems that the bug has been fixed. Edit the test script to disable workaround.");}assert_script_run(q{fedora-third-party list --csv --columns=name,type | grep -q .});maybe a comment to explain exactly what this does would be good? it's not immediately obvious, I don't think.
84942abe18ab4a2989achandle_welcome_screento clean the Tour 545de3a53a142e061ef77bab70afd97bab70afd92ea3815c2dRebased, tidied and fixed the mismatched needle tag between the test and the needle itself.
AI Code Review
📋 MR Summary
This MR adds a new test suite for GNOME Initial Setup, which resets user state, navigates the setup wizard, and verifies settings via CLI post-installation.
aaa_setup.pmto kill the current user session and reset state.gnome_initial_setup.pmto navigate the GIS screens and configure non-default settings.gnome_initial_setup_post.pmto assert that settings were successfully applied.console_loadkeys_usinutils.pmto handle Czech layouts.Detailed Code Review
The test correctly configures settings and validates them. The author successfully applied prior feedback regarding user session cleanup and the keyboard layout loading workaround. However, a discussed improvement to validate that no mandatory setup screens were skipped has not been implemented. In
gnome_initial_setup.pm, the code loops blindly and clicks through screens as they appear, risking a false positive if a screen is skipped. Also, themilestone => 1flag remains ingnome_initial_setup.pm, contrary to the commit history stating it was removed. Error handling in password verification logic has a minor shell issue.📂 File Reviews
📄 `tests/applications/gnome-initial-setup/gnome_initial_setup.pm` - Handles the GIS UI flow by continuously checking the active screen.
test_flagssubroutine still returnsmilestone => 1, even though the PR description and commit history state it was removed.test_flags.📄 `tests/applications/gnome-initial-setup/gnome_initial_setup_post.pm` - Verifies that settings chosen in GIS are reflected in the system.
assert_password, passing$passwordvia<<<without proper escaping could cause issues if the password contains special characters.📄 `lib/utils.pm` - Updates keyboard layout logic to handle Czech layouts.
get_var('LAYOUT' eq 'czech')is passing a boolean toget_varinstead of checking the returned value.get_var('LAYOUT').✅ Summary
get_var('LAYOUT' eq 'czech')inutils.pm.gnome_initial_setup.pmto prevent false positive passes if screens are skipped.milestone => 1fromgnome_initial_setup.pm'stest_flags.$passwordingnome_initial_setup_post.pmto prevent shell interpretation issues with special characters.🤖 AI Code Review | Generated with ai-code-review | Model:
gemini-3.1-pro-preview⚠️ AI-generated suggestions may be incorrect. Verify before applying. Not a replacement for human review.
AI review looks about right - indeed you didn't do anything directly about the 'we pass even if expected pages are missing' problem (though see discussion above), and we should remove
milestonefromgnome_initial_setup.pmas well asaaa_setup.pm. The password quoting thing is not a practical issue.Ah, yeah, so I ran this in staging and there's a bigger issue: we recently tweaked things again so you don't see the language and keyboard layout screens in a typical Fedora Workstation install (the idea is that you made those choices in the installer so you don't need to make them again).
Easiest thing to do is probably to just rework this not to expect those two screens any more. At some point they might come back and be shown on live boot before the installer runs, but...we'll deal with that when it happens.
000c1559b3e7e679561dYes, that's correct, at the time of the second AI review, I hadn't done anything to it. Now,
milestonefrom the test flags.This is not a problem, because the test would only complain if there was a screen it does not know. It does not mind when there is a screen missing (~ therefore we now record the screens shown and compare with a required list)
I am checking for the type of the welcome page. Either it is the language settings welcome page, or it is the real one where we actually welcome someone. The test can handle both.
I added a condition that switches off language and layout checking as there is no way to switch to Czech now. It can be flipped back by changing an assigned variable to 1 if needed.
e7e679561db60b5ba570The follow-up test (of the failed origin) now passes.
AI Code Review
📋 MR Summary
This MR introduces a new test suite for GNOME Initial Setup (GIS), verifying that system and user settings selected during the first-boot wizard are correctly applied. It resets an existing user session to trigger GIS, navigates the UI, and subsequently validates the configured settings through CLI assertions.
aaa_setup.pmto clear out user sessions and trigger the GNOME Initial Setup on reboot.gnome_initial_setup.pmto programmatically navigate through the GIS wizard, selecting specific values (language, timezone, privacy, user credentials).gnome_initial_setup_post.pmto verify via CLI that the settings chosen during the wizard are actually applied to the system and user session.loginctl kill-useranduserdel -ralters system state significantly. The test interacts heavily with graphical elements, which could introduce flakiness if UI updates occur, though the overall risk to the production application is low.Detailed Code Review
The test suite is structurally sound, dividing the process into a clean setup, execution, and post-verification phase. The implementation successfully acts upon the previously discussed agreements, like relying on the
handle_welcome_screenfunction and skipping milestone rollbacks.However, there are a few issues identified.
utils::console_loadkeys_uscontains a typo (loadkezs) when interacting with Czech layouts. Additionally, insidegnome_initial_setup_post.pm, testing the user's password usingsuand piping input may be flaky or hang ifsudoes not behave identically across environments. A safer approach for user authentication checking should be considered.Also,
tests/applications/gnome-initial-setup/gnome_initial_setup_post.pmperforms redundantlocale_checkevaluation and output.📂 File Reviews
📄 `lib/utils.pm` - Updates keyboard layout logic to handle Czech layouts.
loadkezstoloadkeysunless you are explicitly compensating for a raw scancode mapping issue, in which case it should be documented. Given standard OpenQAscript_runbehavior, it typically sends strings assuming the current layout mapping.📄 `tests/applications/gnome-initial-setup/gnome_initial_setup_post.pm` - Verifies settings applied during the GIS wizard via CLI checks.
Major [Logic]: The
assert_passwordfunction uses a here-string (<<<) piped tosu, which can be unreliable.suoften requires a PTY to read the password and may ignore standard input entirely, causing the test to hang waiting for a password prompt.sudowith-Sif the user is in the wheel group. Alternatively, rely on OpenQA'senter_cmdandwait_serialif interacting with the prompt directly.Minor [Complexity]: The
$locale_checkvariable is hardcoded to 0 and evaluated twice in the script to skip tests, resulting in duplicate logging.✅ Summary
loadkezs->loadkeysinlib/utils.pm.assert_passwordingnome_initial_setup_post.pmto reliably authenticate the user without relying on piping tosu.if ($locale_check == 0)blocks ingnome_initial_setup_post.pm.🤖 AI Code Review | Generated with ai-code-review | Model:
gemini-3.1-pro-preview⚠️ AI-generated suggestions may be incorrect. Verify before applying. Not a replacement for human review.
b60b5ba5703f0719f7c73f0719f7c7b7bef16137Noticed a couple of issues with this on last Rawhide:
@adamwill wrote in #517 (comment):
Yeah, I'll try to wait some time. Have not seen anything like that before.
Yes, the slider is missing. It was not missing on yesterday's Rawide, if I can recall correctly. Or maybe Workstation has finally dismantled Abrt and they removed the option?
It seems that on Silverblue, this is expected. I guess I will have to check for the other one then.

2ee527fd3a2469102579246910257978e6deae3aSome more notes inline, thanks! Note I rebased this and added a tweak to the
required_screensstuff which makes it shorter and makes tidy happy.@ -383,0 +381,4 @@elsif (get_var('LANGUAGE') eq 'czech' || get_var('LAYOUT') eq 'czech') {script_run('loadkezs us', 0);}wait_still_screen(3);this makes us wait even if we didn't actually have to run loadkeys. avoiding that is why we were repeating it in each block. this function is called unconditionally; we just expect it to do nothing and immediately return unless a relevant
LANGUAGE/LAYOUTis set.Hmm, yes, you are right.
@ -0,0 +8,4 @@sub run {my $self = shift;my $username = get_var("USER_LOGIN") // "test";get_varimplements default values internally, just use that:get_var('USER_LOGIN', 'test')@ -0,0 +10,4 @@sub get_screen {# Returns the name of the current screen based on its content.my %tags = (there's probably a really cool way to do this with
mapor somethingI am not sure if I know how to do it.
@ -0,0 +39,4 @@set_var('LANGSELECT', '1');}else {assert_and_click('gis_button_start_setup');we already have a
start_setuptag and several needles for it, it's used in the existing g-i-s handler, please get rid of this duplicate needle and use the existing one.@ -0,0 +87,4 @@sub handle_credentials {my ($realname, $username) = @_;type_very_safely($realname);send_key("tab");maybe a
wait_screen_changewrapper would be a good idea?@ -0,0 +135,4 @@# Check if we see the "Start Setup" button and in# that case, click on it, otherwise do the language# selection.if (check_screen('gis_button_start_setup', timeout => 10)) {see earlier note about this being a dupe needle.
@ -0,0 +165,4 @@}for my $screen (@required_screens) {die("GNOME Initial Setup skipped this required screen: $screen") unless $visited_screens{$screen};this will only tell us about one missed required screen, won't it? what if there's more than one?
@ -0,0 +4,4 @@use testapi;use utils;sub assert_cmd_output_eq {why not use
validate_script_output?@ -0,0 +13,4 @@sub run_as_user {my ($user, $cmd) = @_;return script_output(qq{su -l $user -c 'dbus-run-session bash -lc "$cmd"'}, timeout => 60);why is this necessary, as opposed to just using
suthen using normal commands? I guess there's a reason but it'd be good to explain it in a comment here.@ -0,0 +53,4 @@# Switch to the serial console. To run commands successfully,# we need to load english keyboard layout. Otherwise, we cannot# run commands correctly. Note, that the loadkeys command must# be presented in the foreign layout.this comment seems like it's in slightly the wrong place now?
This is not ready yet. Silverblue keeps breaking a lot of stuff, for instance:
suand normal commands.Difficult to say, what is correct and what is not. So I am trying to make the tests pass on both, using conditionals.
Now, the tests pass on both silverblue and workstation:
https://openqa.stg.fedoraproject.org/tests/6325481#details
https://openqa.stg.fedoraproject.org/tests/6325480#details
I tried to do everything you requested. Note, that the commits are not squashed.
This is normal as we don't have that whole thing implemented for non-RPM.
10505ec1414d8e8fccfaAI Code Review
📋 MR Summary
This PR adds a new comprehensive test suite for the GNOME Initial Setup (GIS), verifying that initial wizard choices (language, layout, privacy toggles, users) are correctly applied to the newly created user session.
aaa_setup.pmto clear out the existing user session and force the GIS wizard to trigger on the next boot.gnome_initial_setup.pmto traverse the GIS UI using specific needle interactions.gnome_initial_setup_post.pmto validate the system and user configurations applied by the GIS via shell commands.lib/utils.pmto accommodate loading the US keyboard layout gracefully when a Czech layout is active.supassword prompts) could lead to unexpected hangs or test timeouts if not handled perfectly.Detailed Code Review
Overall, the structured approach to verify the Initial Setup flow step-by-step is logical. However, the post-setup verification script relies on interactive terminal session switching (
su - $user) and injecting passwords viastdintosu, which historically causes issues with terminal behavior and test framework prompt matching. Consolidating shell executions and avoiding interactive privilege changes will significantly increase test reliability.📂 File Reviews
📄 `tests/applications/gnome-initial-setup/aaa_setup.pm` - Handles test preparations by terminating the current user session, deleting the user, and rebooting.
type_string("reboot\n");works, but usingenter_cmd("reboot");or standard framework power actions is generally safer and automatically handles newline appending properly.type_stringwithenter_cmd.📄 `tests/applications/gnome-initial-setup/gnome_initial_setup_post.pm` - Validates that user selections made in the UI were correctly translated into system and user profile settings.
sucommand explicitly reads passwords from/dev/ttyand ignores standard input (like<<< $password). This will cause the test to hang indefinitely waiting for interactive password entry.type_passwordinteractions orsudo -Swhich can read from stdin.enter_cmd("su - $user")and then usingscript_outputcan confuse the testing framework's prompt detection (which usually expects a specific root prompt like#).su -cexecuted from the root shell. This maintains the expected root prompt.✅ Summary
supassword injection exist and must be addressed to prevent the post-verification tests from hanging indefinitely.assert_passwordbecausesuignores stdin for passwords, which will stall the execution.run_as_userto avoid interactivesusessions that break openQA's prompt matching algorithm.enter_cmdinstead of rawtype_stringfor reboot execution.utils.pmexplaining whyloadkezsis used for the Czech layout (z/y swapping) for future maintainers.🤖 AI Code Review | Generated with ai-code-review | Model:
gemini-3.1-pro-preview⚠️ AI-generated suggestions may be incorrect. Verify before applying. Not a replacement for human review.
I've rebased this and deployed on staging to see what happens overnight. Please take a look at the AI review's suggestions but only do things that you think make sense.
cedbfc8f0728ed6961f5AI Code Review
📋 MR Summary
Adds a new test suite to verify GNOME Initial Setup functionality by simulating user flow and validating the resulting system state.
aaa_setup.pmto reset the user session and trigger GNOME Initial Setup on reboot.gnome_initial_setup.pmto drive the GUI interactions through the initial setup wizard.gnome_initial_setup_post.pmto verify system and user settings applied by the setup.lib/utils.pmto handle Czech keyboard layout mapping.Detailed Code Review
The implementation generally meets the intended goal of covering the GNOME Initial Setup flow. However, there are potential issues with how specific commands are asserted or parsed, particularly when verifying the user's password using
sudoand managing list filtering. Additionally, the approach to checking language states post-setup has hardcoded environment variables that could cause silent failures if the system outputs unexpected strings. The structure looks well segmented into setup, execution, and verification steps.📂 File Reviews
📄 `tests/applications/gnome-initial-setup/gnome_initial_setup.pm` - Drives the GNOME Initial Setup wizard using predefined UI elements.
Major [Bug]: In
handle_privacy, the code usescheck_screenwith a timeout of 2 seconds to check if a setting is enabled. However, if the screen doesn't match immediately due to slight rendering delays, it logs a failure but continues execution, potentially clicking the wrong UI elements or failing subsequent checks.Minor [Logic]: The logic to handle required screens in
runbuilds an arrayqw(welcome privacy timezone credentials password)and optionally adds 'software'. It later checks againstvisited_screens. If the test breaks out of the loop early on 'final', it correctly reports missing screens. However, ifget_screenfails to match anything, the script dies immediately and the missing screens logic is bypassed entirely.📄 `tests/applications/gnome-initial-setup/gnome_initial_setup_post.pm` - Validates the system and user configurations applied during GNOME Initial Setup.
assert_passwordfunction echoes the password directly into asudocommand without properly sanitizing or quoting the variables. If$passwordor$usercontained shell metacharacters, it could result in command injection.script_run.grep -xcommand requires an exact match for the whole line. The output oflocalectl statusmight have trailing spaces or slightly different formatting across versions, making the strict regexgrep -x 'LANG=cs_CZ.UTF-8' /etc/locale.confprone to failure.validate_script_output.✅ Summary
assert_passwordby quoting shell variables.grep -xmatching when parsing configuration files and command outputs.🤖 AI Code Review | Generated with ai-code-review | Model:
gemini-3.1-pro-preview⚠️ AI-generated suggestions may be incorrect. Verify before applying. Not a replacement for human review.
The AI review is doing stuff in the circle. I would refrain from listening to it any more. We want to check for exact strings, I think. Also, I do not know why AI would not report this earlier if that was so severe. By the way the
grep -xcheck was suggested as improvement by the same AI.I will only bump the
check_screentimeout just to be on the safe side.28ed6961f5a145cda3b9As I said, feel free to ignore things the AI says that don't make sense. Use your judgment :D Making it happy isn't a requirement. It's just an input.
a145cda3b96aaf2fe58dIt looks like it now fails on Workstation because abrt is gone from Workstation too now.
9122affa2cd02ad52c7dI fixed that. But now I'm noticing something else. It seems like we have duplicated versions of a lot of the needles - one in English, one in Czech. On further review this seems to be because we get the language selection screen on Silverblue, so we can configure Czech, but on Workstation we don't, so we are stuck in English.
I think the fact that the language and layout pages are shown in Silverblue may be unintended, so I've filed https://github.com/fedora-silverblue/issue-tracker/issues/700 . If that gets changed, we might have to reconsider this further. Do we try to force the appearance of the language and layout pages so we can test them, or do we say it's fine if they're broken as we can't encounter them on any typical Fedora install path any more? Do we still want to test the rest of g-i-s in Czech or is testing in English OK?
Also, @catanzaro says he's going to re-introduce live mode g-i-s soon, so we have that to look forward to.
Notably, that's where language and keyboard layout selection will move to. So your tests will need to be reworked, but maybe don't completely delete them.
d02ad52c7daa5ba3ba1baa5ba3ba1b951618b9c0951618b9c0bc098e5b57bc098e5b57e37761af0de37761af0de2599185d3It seems it's gonna stay this way for at least a bit, so I think we can probably merge this as-is and refine it later.