-
Notifications
You must be signed in to change notification settings - Fork 17
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
webui: change hardcoded installation phases to new API #45
webui: change hardcoded installation phases to new API #45
Conversation
@adamkankovsky take in mind that you can trigger against your anaconda PR tests like this: bots/tests-trigger --allow 45 fedora-rawhide-boot/anaconda-pr-5329@rhinstaller/anaconda-webui |
bafe0b3
to
8fb1b88
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you also maybe add some tests for the expected categories?
909cce9
to
ef9172f
Compare
ef9172f
to
55fa7eb
Compare
b33f77e
to
1131371
Compare
1131371
to
ebccd2c
Compare
8222da0
to
867ec85
Compare
867ec85
to
fa588a6
Compare
25e74d6
to
5952676
Compare
1a1d45a
to
1169762
Compare
@@ -40,6 +40,16 @@ class TestInstallationProgress(anacondalib.VirtInstallMachineCase): | |||
i.reach(i.steps.REVIEW) | |||
i.begin_installation() | |||
|
|||
with b.wait_timeout(300): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This yells flaky to me :D Is there some magic that makes sure that these messages will be seen on time and reliably?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's really waiting for the installation messages to appear. The installation can easily take 5mins to finish, so show all messages.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no magic behind this, unfortunately. There is only a timeout constant shifted due to too long installation phase, which is tested so that in case of correct operation it will not end with wait_in_text timeout. But of course there is some flake involved, but I can't think of a better way to test the installation phase
packaging/anaconda-webui.spec.in
Outdated
%global anacondacorever 40.20 | ||
%if %{?fedora} < 40 | ||
# Web UI is not supported on Fedora < 40 | ||
%global anacondacorever 1000 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is to require non-existing version, thus kill the install 🤔 I am not a packager so no idea if there is better way and quick read through guidelines did not yield any results so I guess all good?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So often we build on some other Fedora the srpms, and not having the variable defined kills rpmbuild. I dont know what's the 'correct' approach here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It "kills" (i.e. fails) rpmbuild because it can't parse Requires: anaconda-core >= %{anacondacorever}
then? That's strange, it shouldn't care about this at srpm build time. But if/as it apparently does, a cleaner way would be either to conditionalize that Requires like on if 0%{?anacondacorever}
(more correct and cleaner), or setting the default value to 0
(sloppy, but easier). I suppose you don't want to render this uninstallable on earlier Fedoras if you try and build them there?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I would like to make this uninstallable for < 40. I would like to allow people to build it for >= 40, even though it's decided to not be the default installer for 40, still it makes sense to enable that for testing purposes.
packaging/anaconda-webui.spec.in
Outdated
%global anacondacorever 40.22.2 | ||
%endif | ||
|
||
%if %{?fedora} >= 41 | ||
%global anacondacorever 41.2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you explain why these versions like this? So I assume this PR does not work with version less than 40.22.2
. But why 41.2
already in F41 (which only branched now). I assume that Fedora versions their packages by fedora version? (that must be hella confusing! :D) If so, than 40.22.2 == 41.2? So 41.0 and 41.1 do not work?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Anaconda tags the different branches with different tags, which start with the Fedora version number. So branch master, is now targetting Fedora 41, which and 41.2 is the second release, on that branch after the branching of Fedora 40.
Based on Fesco's decision, about Fedora 40 not containing Web UI, I will remove the conditional here, as it's not needed.
@marusak and yes it's a bit confusing coming from cockpit with single number as tag.
1169762
to
c267267
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me.
blocker: rhinstaller/anaconda#5329