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

Run an installation task to install a language #1502

Conversation

poncovka
Copy link
Contributor

@poncovka poncovka commented Jun 7, 2018

The lang command will run a remote installation task to install
a language on the installed system.

@poncovka poncovka added the master Please, use the `f39` label instead. label Jun 7, 2018
Copy link
Member

@jkonecny12 jkonecny12 left a comment

Choose a reason for hiding this comment

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

Please look on my notes below. Otherwise I like the new tasks solution.

@@ -0,0 +1,40 @@
#
Copy link
Member

Choose a reason for hiding this comment

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

What about having all the tasks in modules/<module_name>/tasks subfolder. So in this case it would be modules/localization/tasks/installation.py?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not sure about this. I would prefer to sort tasks by their functionality. For example in the storage module, you don't want to have all tasks in pyanaconda.modules.storage.tasks, but you might want to keep them close to other related files, for example the bootloader tasks should be in pyanaconda.modules.storage.bootloader.

Copy link
Member

Choose a reason for hiding this comment

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

And what about sorting then inside of that subfolder?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know. What do @rvykydal and @M4rtinK think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We agreed that tasks will not be in a subfolder tasks.

return "Configure language"

def run(self):
write_language_configuration(self._lang, self._sysroot)
Copy link
Member

Choose a reason for hiding this comment

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

Why the write_language_configuration() method is not part of this task? I don't think it should be run outside of the task or should it be?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I can move it there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

@jkonecny12
Copy link
Member

jenkins, test this please

@poncovka poncovka force-pushed the modularization-devel-install_lang branch from 1ada473 to 7e17797 Compare June 11, 2018 11:04
The lang command will run a remote installation task to install
a language on the installed system.
@poncovka poncovka force-pushed the modularization-devel-install_lang branch from 7e17797 to c5ae710 Compare June 20, 2018 15:24
Added new tests for the localization module changes and the task.
@poncovka poncovka force-pushed the modularization-devel-install_lang branch from c5ae710 to 733a488 Compare June 20, 2018 15:27
@poncovka
Copy link
Contributor Author

Updated: Created a specific exception and added new tests for the module and the task.

Copy link
Member

@jkonecny12 jkonecny12 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 :).

@poncovka poncovka merged commit 1189670 into rhinstaller:modularization-devel Jun 21, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
master Please, use the `f39` label instead.
2 participants