Migrate the kickstart script commands #5739

adamkankovsky · 2024-07-03T06:22:01Z

Working with: #5775
Thanks @rvykydal.

pep8speaks · 2024-07-03T06:22:09Z

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-09-19 14:31:42 UTC

jkonecny12

Otherwise looks good to me so far.

jkonecny12 · 2024-07-11T10:57:55Z

pyanaconda/modules/runtime/scripts/Makefile.am

+scriptsdir = $(pkgpyexecdir)/modules/runtime/scripts
+dist_scripts_DATA = $(wildcard $(srcdir)/*.py)
+
+MAINTAINERCLEANFILES = Makefile.in


Suggested change

MAINTAINERCLEANFILES = Makefile.in

MAINTAINERCLEANFILES = Makefile.in

Add missing newline at the end of the file.

jkonecny12 · 2024-07-11T11:00:07Z

pyanaconda/modules/runtime/scripts/Makefile.am

+# along with this program. If not, see <http://www.gnu.org/licenses/>.
+
+pkgpyexecdir = $(pyexecdir)/py$(PACKAGE_NAME)
+scriptsdir = $(pkgpyexecdir)/modules/runtime/scripts


I have a feeling that his will not work correctly. There is already scriptsdir variable (scripts/Makefile.am) and I have a feeling these are global. I'm not 100% sure how ti behaves so please make sure that your commit won't break automake and both directories are processed correctly by automake.

So far everything works as it should and there is no problem with the module

rvykydal · 2024-07-18T09:31:07Z

I'll just add link to my WIP before @adamkankovsky took over: #5775, may be useful for reference. (I used Katerina's WIP in the same way).
In my notes in 9d3fb85 I say %pre is most challenging, so maybe it is good to work on it first, like Adam does.

adamkankovsky · 2024-07-24T10:32:46Z

/kickstart-test --testtype smoke

adamkankovsky · 2024-07-24T13:23:31Z

/kickstart-test --testtype smoke

jkonecny12

Looks great otherwise.
Also please take a look on coverage. I'm surprised that for such a change we don't have any new tests.

jkonecny12 · 2024-07-24T14:18:43Z

pyanaconda/core/kickstart/scripts.py

+ execution.
+ Output is logged by the program logger, the path specified by --log
+ or to /tmp/ks-script-\\*.log
+ # TODORV return


I guess this is leftover.

jkonecny12 · 2024-07-24T14:21:05Z

pyanaconda/core/kickstart/scripts.py

@@ -0,0 +1,80 @@
+#
+# Commond utilities for working with scripts


Commond is typo?

jkonecny12 · 2024-07-24T14:25:51Z

pyanaconda/core/kickstart/specification.py

+ self.scripts = []
+


Why is this required?

Yes, because sections for scripts don't have a data parser like packages does, and sections themselves don't have an action to create an array for scitps. Therefore it is necessary to create it manually. It looks quite strange, but I spent a lot of time on this parsing and unfortunately I didn't come up with anything better using pykickstart

jkonecny12 · 2024-07-24T14:38:07Z

pyanaconda/modules/runtime/scripts/scripts.py

+ for script in self._scripts:
+ if script.type == self._script_type:
+ if script.type == KS_SCRIPT_POST:
+ script.run(conf.target.system_root)


If I'm reading this correctly then you are saying that post script is always run in chroot. That is not correct it depends on --nochroot parameter.

This is only the path which is used if --chroot is set. Default is always "/".

jkonecny12 · 2024-07-24T14:43:57Z

pyanaconda/modules/runtime/scripts/scripts.py

+__all__ = ["ScriptsModule"]
+
+
+class RunScriptsTask(Task):


Please separate this class into another file.

adamkankovsky · 2024-08-20T08:35:32Z

/kickstart-test image-deployment-2

rvykydal · 2024-09-03T08:39:21Z

/kickstart-test image-deployment-2

rvykydal · 2024-09-03T09:16:14Z

/kickstart-test --skip-testtypes whatever

rvykydal · 2024-09-03T09:28:50Z

I wonder if we can (as a follow-up, not requiring in scope of this PR) add some kickstart tests testing the scripts. In general, post and pre would be perhaps well tested by existing tests as they are using them heavily, but we may want to test also

various options (as erroronfail)
if the scripts are logged, failures reported as expected
%onerror and %traceback
It can be difficult with current kickstart tests capabilities but we could have at least MANUAL tests (checking the result, ie error dialog appearing by developer). For development, reference, and maybe future automation.

pyanaconda/modules/common/errors/runtime.py

rvykydal · 2024-09-03T09:58:59Z

/kickstart-test --testtype smoke

rvykydal · 2024-09-03T11:51:01Z

/kickstart-test --testtype smoke

The tests are failing.

10:16:09,148 WARNING org.fedoraproject.Anaconda.Modules.Runtime:INFO:anaconda.core.threads:Thread Failed: AnaTaskThread-RunScriptsTask-2 (140199159924416)
10:16:09,176 WARNING org.fedoraproject.Anaconda.Modules.Runtime:ERROR:anaconda.modules.common.task.task:Thread AnaTaskThread-RunScriptsTask-2 has failed: Traceback (most recent call last):
10:16:09,176 WARNING org.fedoraproject.Anaconda.Modules.Runtime:  File "/usr/lib64/python3.13/site-packages/pyanaconda/core/threads.py", line 280, in run
10:16:09,176 WARNING org.fedoraproject.Anaconda.Modules.Runtime:    threading.Thread.run(self)
10:16:09,176 WARNING org.fedoraproject.Anaconda.Modules.Runtime:    ~~~~~~~~~~~~~~~~~~~~^^^^^^ 
10:16:09,176 WARNING org.fedoraproject.Anaconda.Modules.Runtime:  File "/usr/lib64/python3.13/threading.py", line 992, in run
10:16:09,176 WARNING org.fedoraproject.Anaconda.Modules.Runtime:    self._target(*self._args, **self._kwargs)
10:16:09,176 WARNING org.fedoraproject.Anaconda.Modules.Runtime:    ~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
10:16:09,176 WARNING org.fedoraproject.Anaconda.Modules.Runtime:  File "/usr/lib64/python3.13/site-packages/pyanaconda/modules/common/task/task.py", line 94, in _thread_run_callback
10:16:09,176 WARNING org.fedoraproject.Anaconda.Modules.Runtime:    self._task_run_callback()
10:16:09,176 WARNING org.fedoraproject.Anaconda.Modules.Runtime:    ~~~~~~~~~~~~~~~~~~~~~~~^^
10:16:09,176 WARNING org.fedoraproject.Anaconda.Modules.Runtime:  File "/usr/lib64/python3.13/site-packages/pyanaconda/modules/common/task/task.py", line 107, in _task_run_callback
10:16:09,176 WARNING org.fedoraproject.Anaconda.Modules.Runtime:    self._set_result(self.run())
10:16:09,176 WARNING org.fedoraproject.Anaconda.Modules.Runtime:                     ~~~~~~~~^^
10:16:09,176 WARNING org.fedoraproject.Anaconda.Modules.Runtime:  File "/usr/lib64/python3.13/site-packages/pyanaconda/modules/runtime/scripts/runtime.py", line 51, in run
10:16:09,176 WARNING org.fedoraproject.Anaconda.Modules.Runtime:    lineno, err = script.run(conf.target.system_root)
10:16:09,176 WARNING org.fedoraproject.Anaconda.Modules.Runtime:    ^^^^^^^^^^^
10:16:09,176 WARNING org.fedoraproject.Anaconda.Modules.Runtime:TypeError: cannot unpack non-iterable NoneType object

rvykydal · 2024-09-03T12:29:09Z

I was also wondering if we should pass failing (erroronfail) script via return values and handle / raise in client rather then via dbus error / exception. But I am not deep enough in the matter to be able to give reasons.

adamkankovsky · 2024-09-10T07:55:16Z

/kickstart-test --testtype smoke

adamkankovsky temporarily deployed to gh-cockpituous July 3, 2024 06:22 — with GitHub Actions Inactive

adamkankovsky marked this pull request as draft July 3, 2024 06:22

github-actions bot added the f41 label Jul 3, 2024

jkonecny12 reviewed Jul 11, 2024

View reviewed changes

adamkankovsky force-pushed the migration-kickstart-scripts branch from c13788b to b039904 Compare July 22, 2024 13:41

adamkankovsky temporarily deployed to gh-cockpituous July 22, 2024 13:41 — with GitHub Actions Inactive

adamkankovsky force-pushed the migration-kickstart-scripts branch from b039904 to 2956709 Compare July 23, 2024 06:37

adamkankovsky temporarily deployed to gh-cockpituous July 23, 2024 06:37 — with GitHub Actions Inactive

adamkankovsky force-pushed the migration-kickstart-scripts branch from 2956709 to d7507b4 Compare July 23, 2024 08:38

adamkankovsky temporarily deployed to gh-cockpituous July 23, 2024 08:38 — with GitHub Actions Inactive

adamkankovsky force-pushed the migration-kickstart-scripts branch from d7507b4 to d5d17d6 Compare July 23, 2024 13:07

adamkankovsky temporarily deployed to gh-cockpituous July 23, 2024 13:07 — with GitHub Actions Inactive

adamkankovsky force-pushed the migration-kickstart-scripts branch from d5d17d6 to 305d912 Compare July 23, 2024 13:12

adamkankovsky had a problem deploying to gh-cockpituous July 23, 2024 13:12 — with GitHub Actions Failure

adamkankovsky marked this pull request as ready for review July 23, 2024 13:15

adamkankovsky force-pushed the migration-kickstart-scripts branch from 305d912 to 42c0b63 Compare July 24, 2024 07:43

adamkankovsky temporarily deployed to gh-cockpituous July 24, 2024 07:43 — with GitHub Actions Inactive

adamkankovsky changed the title ~~Migrate the %pre script~~ Migrate the kickstart script commands Jul 24, 2024

adamkankovsky requested a review from jkonecny12 July 24, 2024 10:32

adamkankovsky force-pushed the migration-kickstart-scripts branch from 42c0b63 to 1c4f683 Compare July 24, 2024 12:58

adamkankovsky temporarily deployed to gh-cockpituous July 24, 2024 12:59 — with GitHub Actions Inactive

jkonecny12 requested changes Jul 24, 2024

View reviewed changes

adamkankovsky force-pushed the migration-kickstart-scripts branch from 1c4f683 to 3184a63 Compare July 24, 2024 15:22

adamkankovsky temporarily deployed to gh-cockpituous July 24, 2024 15:22 — with GitHub Actions Inactive

adamkankovsky force-pushed the migration-kickstart-scripts branch from 3184a63 to 46062e2 Compare July 24, 2024 15:27

adamkankovsky had a problem deploying to gh-cockpituous July 24, 2024 15:27 — with GitHub Actions Failure

adamkankovsky temporarily deployed to gh-cockpituous August 19, 2024 15:45 — with GitHub Actions Inactive

adamkankovsky force-pushed the migration-kickstart-scripts branch from 7efbab0 to 03d4236 Compare August 20, 2024 06:00

adamkankovsky temporarily deployed to gh-cockpituous August 20, 2024 06:00 — with GitHub Actions Inactive

adamkankovsky force-pushed the migration-kickstart-scripts branch from 03d4236 to 387b031 Compare August 20, 2024 06:19

adamkankovsky temporarily deployed to gh-cockpituous August 20, 2024 06:19 — with GitHub Actions Inactive

adamkankovsky force-pushed the migration-kickstart-scripts branch from 387b031 to b0d383b Compare August 20, 2024 08:01

adamkankovsky temporarily deployed to gh-cockpituous August 20, 2024 08:01 — with GitHub Actions Inactive

rvykydal requested a review from M4rtinK September 3, 2024 08:38

rvykydal reviewed Sep 3, 2024

View reviewed changes

pyanaconda/modules/common/errors/runtime.py Outdated Show resolved Hide resolved

adamkankovsky force-pushed the migration-kickstart-scripts branch from b0d383b to a7facef Compare September 10, 2024 07:26

adamkankovsky temporarily deployed to gh-cockpituous September 10, 2024 07:26 — with GitHub Actions Inactive

adamkankovsky force-pushed the migration-kickstart-scripts branch from a7facef to 1e5d0aa Compare September 10, 2024 07:45

adamkankovsky temporarily deployed to gh-cockpituous September 10, 2024 07:45 — with GitHub Actions Inactive

adamkankovsky added 2 commits September 19, 2024 09:08

Migrate the %pre-install and %post script

9cea1cc

Migrate the %onerror and %traceback script

d784704

adamkankovsky force-pushed the migration-kickstart-scripts branch from 1e5d0aa to 6648dff Compare September 19, 2024 07:20

adamkankovsky temporarily deployed to gh-cockpituous September 19, 2024 07:20 — with GitHub Actions Inactive

adamkankovsky temporarily deployed to gh-cockpituous September 19, 2024 07:33 — with GitHub Actions Inactive

adamkankovsky mentioned this pull request Sep 19, 2024

Tests for kickstart script commands rhinstaller/kickstart-tests#1302

Open

Test for kickstart scripts

f8719ac

adamkankovsky force-pushed the migration-kickstart-scripts branch from 6648dff to f8719ac Compare September 19, 2024 14:31

adamkankovsky temporarily deployed to gh-cockpituous September 19, 2024 14:31 — with GitHub Actions Inactive

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate the kickstart script commands #5739

Migrate the kickstart script commands #5739

adamkankovsky commented Jul 3, 2024 •

edited

Loading

pep8speaks commented Jul 3, 2024 •

edited

Loading

jkonecny12 left a comment

jkonecny12 Jul 11, 2024

jkonecny12 Jul 11, 2024

jkonecny12 Jul 11, 2024

adamkankovsky Jul 23, 2024

rvykydal commented Jul 18, 2024 •

edited

Loading

adamkankovsky commented Jul 24, 2024

adamkankovsky commented Jul 24, 2024

jkonecny12 left a comment

jkonecny12 Jul 24, 2024

jkonecny12 Jul 24, 2024

jkonecny12 Jul 24, 2024

adamkankovsky Jul 24, 2024

jkonecny12 Jul 24, 2024

adamkankovsky Jul 24, 2024

jkonecny12 Jul 24, 2024

adamkankovsky commented Aug 20, 2024

rvykydal commented Sep 3, 2024

rvykydal commented Sep 3, 2024

rvykydal commented Sep 3, 2024

rvykydal commented Sep 3, 2024

rvykydal commented Sep 3, 2024 •

edited

Loading

rvykydal commented Sep 3, 2024

adamkankovsky commented Sep 10, 2024

	MAINTAINERCLEANFILES = Makefile.in
	MAINTAINERCLEANFILES = Makefile.in

		@@ -0,0 +1,80 @@
		#
		# Commond utilities for working with scripts

Migrate the kickstart script commands #5739

Are you sure you want to change the base?

Migrate the kickstart script commands #5739

Conversation

adamkankovsky commented Jul 3, 2024 • edited Loading

pep8speaks commented Jul 3, 2024 • edited Loading

Comment last updated at 2024-09-19 14:31:42 UTC

jkonecny12 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rvykydal commented Jul 18, 2024 • edited Loading

adamkankovsky commented Jul 24, 2024

adamkankovsky commented Jul 24, 2024

jkonecny12 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamkankovsky commented Aug 20, 2024

rvykydal commented Sep 3, 2024

rvykydal commented Sep 3, 2024

rvykydal commented Sep 3, 2024

rvykydal commented Sep 3, 2024

rvykydal commented Sep 3, 2024 • edited Loading

rvykydal commented Sep 3, 2024

adamkankovsky commented Sep 10, 2024

adamkankovsky commented Jul 3, 2024 •

edited

Loading

pep8speaks commented Jul 3, 2024 •

edited

Loading

rvykydal commented Jul 18, 2024 •

edited

Loading

rvykydal commented Sep 3, 2024 •

edited

Loading