Hide internal qubes from selection#291
Conversation
ben-grande
force-pushed
the
preload-hide
branch
from
c6cae0f to
fbd3ec5
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #291 +/- ##
==========================================
- Coverage 92.98% 92.97% -0.02%
==========================================
Files 64 64
Lines 13291 13312 +21
==========================================
+ Hits 12359 12377 +18
- Misses 932 935 +3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
openQArun TEST=system_tests_gui_tools MACHINE=64bit |
1 similar comment
|
openQArun TEST=system_tests_gui_tools MACHINE=64bit |
|
There is a conflict now. |
ben-grande
force-pushed
the
preload-hide
branch
from
fbd3ec5 to
0980a25
Compare
By the amount of times I seen you type this, maybe it is a time to have a bot do that. For easy identification, it could tag/label, but for notifications, PR authors need to receive a comment to be notified. |
marmarta
left a comment
marmarta
left a comment
There was a problem hiding this comment.
Given that you add this filtering to every/almost every dropdown, I think it might be more sensible to add the check to the VMListModeler and if needed, add a parameter to allow internal for some select cases.
Also, what will happen if any of those properties/features/etc/ is set to an existing internal qube? I think in such a case the qube should be shown, but not selectable? (also add tests for such a case, right now I think Weird Shit Will Happen)
| return vm.klass != "TemplateVM" | ||
| return ( | ||
| vm.klass != "TemplateVM" | ||
| and not getattr(vm, "template_for_dispvms", False) |
There was a problem hiding this comment.
why not? I don't see a reason why this should be prohibited
There was a problem hiding this comment.
(and the same comment in other places you add this particular condition; btw, for such choices, this really should be justified in the commit message, because you are adding a completely different condition while saying in the commit message you are doing something else)
There was a problem hiding this comment.
Disposable templates shouldn't be used directly, they are not supposed to be service qubes.
Will fix the commit message.
There was a problem hiding this comment.
Ok, but you added this rule to only some select kind of options without any pattern I can discern?
There was a problem hiding this comment.
Anyway, I remain unconvinced this is a sensible change, and I don't like smuggling it with the 'internal' change.
There was a problem hiding this comment.
Well, I'm not approving it unless I am convinced this is a logical change and the dropdowns you selected make sense. Please don't reply to requests for justification with "I'm just doing to sensible thing" - explain why this is sensible. Why those dropdowns and not others? Why not default netvm for example? Hiding internal qubes from GUI makes sense because they are not supposed to be touched by the GUI at all. Hiding dispvm templates requires actual justification that is not "one developer has a lot of qubes".
There was a problem hiding this comment.
Templates nor disposable templates shouldn't be service qubes. They are not intended to be used directly or by RPC services, they are just intended to create disposables out of it.
Setting a clockvm to a disposable template means that the disposable template always has to be running. This means that we can't refresh the private volume of the disposables if they need updating.
Same thing for devices, you shouldn't attach devices to disposable templates. That is why I am removing it from:
- Device Assignments:
- Device Assignment
- Require Devices
But not from
- Device Assignements:
- Device Attachment policy (deny list)
I also don't want to encourage templates or disposable templates to be a key qube in "Split GPG". To no encourage bad behavior of saving keys to a qube that is shared among disposables.
There was a problem hiding this comment.
Why those dropdowns and not others?
I am doing what I can, from what I have checked...
Why not default netvm for example?
Because it's filter is already fine. It only shows netvms and not disposable template is a netvm by default... although it could become one at user's will.
There was a problem hiding this comment.
I can imagine reasons to attach devices to disposable template (I want to put e.g. a config file in the template and the file is on a usb stick with my backup)...
There was a problem hiding this comment.
Encouraging unsecure behavior is one thing (on clockvm you make a good point), but it is easy to go too far in the other direction and end up all "let's remove the middle click" like certain desktop environments.
OpenQA test summaryComplete test suite and dependencies: https://openqa.qubes-os.org/tests/overview?distri=qubesos&version=4.3&build=2026021104-4.3&flavor=pull-requests Test run included the following:
New failures, excluding unstableCompared to: https://openqa.qubes-os.org/tests/overview?distri=qubesos&version=4.3&build=2026020304-devel&flavor=update
Failed tests8 failures
Fixed failuresCompared to: https://openqa.qubes-os.org/tests/166096#dependencies 33 fixed
Unstable testsDetailsPerformance TestsPerformance degradation:12 performance degradations
Remaining performance tests:98 tests
|
Good, will do that. Didn't imagine things would get to this scale...
Set the clockvm to an internal qube, it loads global config correctly, the qube name appears but not its icon. When listing, it also appears the qube, I don't see this as a issue, as the qube is there anyway. I will try to fix the icon loading, if possible. |
|
Tested hiding internal qubes in VMListModeler and it is working fine, found a way to show the icon. |
check also if it behaves correctly if user changes selection to another qube, changes back to the originally selected internal qube, makes some other changes on the page, presses save (should not change this qube, should work rn? I hope) |
Yes, works fine. |
ben-grande
force-pushed
the
preload-hide
branch
from
0980a25 to
19e5684
Compare
ben-grande
force-pushed
the
preload-hide
branch
from
e52e1b5 to
8e7004d
Compare
|
Video to show the differences, will also help checking if everything is as expected. 2026-02-13-113856.webm |
ben-grande
force-pushed
the
preload-hide
branch
from
8e7004d to
c01fd5c
Compare
| TestService * @anyvm vault allow target=test-blue | ||
| """ | ||
|
|
||
| for qube in test_qapp.domains: |
There was a problem hiding this comment.
we have those nice wrappers to avoid setting default calls, you can set a feature to a value and then update_vm_calls
|
Also please update PR description to cover in detail what the PR does. It's growing to be ever bigger, the changes are pretty opinionated, so I think to get a decent chance of discussing it (and getting other people's input which might be useful) it would help to have a full list of what this actually does in the PR description. It can be copy paste from commit messages (except that the one for U2F is pretty vague still). |
|
After a moment of thought: let's split this. Leave the internal change here (it's pretty indisputable and looks good to me rn) and make a new PR with all the other changes with justification for them, so that it's easier to discuss them and not block the internal change. |
ben-grande
force-pushed
the
preload-hide
branch
from
e8d8451 to
22cda93
Compare
ben-grande
changed the title
ben-grande
force-pushed
the
preload-hide
branch
from
22cda93 to
24e64c2
Compare
Test is failing because it requires: |
|
PipelineRetry |
|
Tests are failing because some recent additions to mock_app, fixing in another PR: |
|
PipelineRetry |
|
PipelineRetryFailed |
|
Passed CI. |
I treat is as approval of the reduced PR. |
| for domain in self.qapp.domains: | ||
| if filter_function and not filter_function(domain): | ||
| continue | ||
| if get_boolean_feature(domain, "internal") and not self.show_internal: |
There was a problem hiding this comment.
Please switch order here, to not fetch internal feature if not needed.
The early load of icon for any qube is to prevent a qube that is not supposed to be listed, to be able to load an icon if it is the default value. Example: set a property to a qube that has the internal feature set, it should be listed in the GUI and should load the icon.
ben-grande
force-pushed
the
preload-hide
branch
from
24e64c2 to
4c5b368
Compare
4 participants
For: QubesOS/qubes-issues#1512
The idea is to hide internal management qubes, disposable templates, and other types of qubes via filter when it shouldn't be displayed on the row box.
Requires:
QubesOS/qubes-core-admin-client#422
TODO: