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.

Sign up for GitHub

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

Jump to bottom

Creating category dbus API for installation phases #5329

Merged
merged 2 commits into from
Feb 19, 2024
Merged

Creating category dbus API for installation phases #5329

merged 2 commits into from
Feb 19, 2024

Conversation

adamkankovsky
Copy link
Contributor

@adamkankovsky adamkankovsky commented Nov 16, 2023 •
edited
Loading

@pep8speaks
Copy link

pep8speaks commented Nov 16, 2023 •
edited
Loading

Hello @adamkankovsky! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2024-02-08 09:10:14 UTC

@adamkankovsky adamkankovsky removed the request for review from KKoukiou November 17, 2023 18:56
@adamkankovsky adamkankovsky mentioned this pull request Nov 17, 2023
Merged
poncovka
poncovka reviewed Dec 5, 2023
View reviewed changes
Copy link
Contributor

@poncovka poncovka left a comment

Choose a reason for hiding this comment

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

I have some suggestions.

pyanaconda/installation.py Outdated Show resolved Hide resolved
pyanaconda/installation.py Outdated Show resolved Hide resolved
pyanaconda/installation.py Outdated Show resolved Hide resolved
pyanaconda/installation.py Outdated Show resolved Hide resolved
pyanaconda/installation.py Outdated Show resolved Hide resolved
pyanaconda/core/constants.py Outdated Show resolved Hide resolved
pyanaconda/core/constants.py Outdated Show resolved Hide resolved
@VladimirSlavik
Copy link
Contributor

Other than that, I agree with having more categories than the UI needs. The UI will have to decode these strings to which icon is active anyway, so there can be more values for the first step, or such.

@VladimirSlavik
Copy link
Contributor

I will have to think a bit about the use of the interface, technically it seems valid, but it's super weird to have a generally-named interface used only in a specific class that implements logic. It's one of these cases where I'd feel a "redundant" ancestor class in between Task and TaskQueue makes sense, so that you can actually pair a class and its interface 1:1.

@poncovka
Copy link
Contributor

poncovka commented Dec 6, 2023

I will have to think a bit about the use of the interface, technically it seems valid, but it's super weird to have a generally-named interface used only in a specific class that implements logic.

Good catch! I have missed that. Yes, the CategoryReportTaskInterface class should be somewhere in the Boss module in a new file with the _interface.py suffix.

@github-actions github-actions bot added the stale label Feb 6, 2024
poncovka
poncovka approved these changes Feb 14, 2024
View reviewed changes
Copy link
Contributor

@poncovka poncovka left a comment

Choose a reason for hiding this comment

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

Otherwise, it looks good to me. Thank you!

tests/unit_tests/pyanaconda_tests/modules/boss/test_install_category.py Outdated

class TestRunInstallation(RunInstallationTask):

def __init__(self, payload, ksdata):
Copy link
Contributor

Choose a reason for hiding this comment

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

You can remove the __init__ method. It doesn't do any extra steps.

@KKoukiou KKoukiou mentioned this pull request Feb 19, 2024
Merged
M4rtinK
M4rtinK approved these changes Feb 19, 2024
View reviewed changes
Copy link
Contributor

@M4rtinK M4rtinK 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 as well. :)

@KKoukiou
Copy link
Contributor

/kickstart-test --testtype smoke

@KKoukiou KKoukiou merged commit b3dfe6a into rhinstaller:master Feb 19, 2024
14 of 17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@poncovka poncovka poncovka approved these changes

@M4rtinK M4rtinK M4rtinK approved these changes

@VladimirSlavik VladimirSlavik Awaiting requested review from VladimirSlavik

Assignees
No one assigned
Labels
f40 webui
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@adamkankovsky @pep8speaks @VladimirSlavik @poncovka @KKoukiou @M4rtinK