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

webui: change hardcoded installation phases to new API #45

Conversation

adamkankovsky
Copy link
Contributor

@KKoukiou
Copy link
Contributor

@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

@adamkankovsky adamkankovsky force-pushed the webui-installation-progress-category-api branch 4 times, most recently from bafe0b3 to 8fb1b88 Compare November 23, 2023 08:47
Copy link
Contributor

@KKoukiou KKoukiou left a 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?

@adamkankovsky adamkankovsky force-pushed the webui-installation-progress-category-api branch 3 times, most recently from 909cce9 to ef9172f Compare November 30, 2023 10:26
@adamkankovsky adamkankovsky force-pushed the webui-installation-progress-category-api branch from ef9172f to 55fa7eb Compare December 6, 2023 13:42
@adamkankovsky adamkankovsky force-pushed the webui-installation-progress-category-api branch 18 times, most recently from b33f77e to 1131371 Compare February 13, 2024 14:37
@adamkankovsky adamkankovsky force-pushed the webui-installation-progress-category-api branch from 1131371 to ebccd2c Compare February 13, 2024 15:07
@adamkankovsky adamkankovsky force-pushed the webui-installation-progress-category-api branch 4 times, most recently from 8222da0 to 867ec85 Compare February 16, 2024 10:58
@KKoukiou KKoukiou force-pushed the webui-installation-progress-category-api branch from 867ec85 to fa588a6 Compare February 19, 2024 16:41
@KKoukiou KKoukiou force-pushed the webui-installation-progress-category-api branch from 25e74d6 to 5952676 Compare February 19, 2024 17:41
@KKoukiou KKoukiou removed the blocked label Feb 19, 2024
@KKoukiou KKoukiou force-pushed the webui-installation-progress-category-api branch 2 times, most recently from 1a1d45a to 1169762 Compare February 19, 2024 18:27
@@ -40,6 +40,16 @@ class TestInstallationProgress(anacondalib.VirtInstallMachineCase):
i.reach(i.steps.REVIEW)
i.begin_installation()

with b.wait_timeout(300):
Copy link
Contributor

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?

Copy link
Contributor

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.

Copy link
Contributor Author

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

%global anacondacorever 40.20
%if %{?fedora} < 40
# Web UI is not supported on Fedora < 40
%global anacondacorever 1000
Copy link
Contributor

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?

Copy link
Contributor

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.

Copy link
Collaborator

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?

Copy link
Contributor

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.

Comment on lines 20 to 21
%global anacondacorever 40.22.2
%endif

%if %{?fedora} >= 41
%global anacondacorever 41.2
Copy link
Contributor

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?

Copy link
Contributor

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.

@KKoukiou KKoukiou force-pushed the webui-installation-progress-category-api branch from 1169762 to c267267 Compare February 20, 2024 09:51
Copy link
Contributor

@rvykydal rvykydal left a 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.

@KKoukiou KKoukiou merged commit bc0e606 into rhinstaller:main Feb 21, 2024
5 of 7 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.

5 participants