AppStreams reposync module #8421

cbbayburt · 2024-03-07T20:13:49Z

A Python module that imports AppStream module metadata from the downloaded .yaml file during repo-sync.

Documentation

Documentation issue was created: https://github.com/SUSE/spacewalk/issues/24091

Test coverage

Unit tests were added

Links

Fixes https://github.com/SUSE/spacewalk/issues/23770

Changelogs

Make sure the changelogs entries you are adding are compliant with https://github.com/uyuni-project/uyuni/wiki/Contributing#changelogs and https://github.com/uyuni-project/uyuni/wiki/Contributing#uyuni-projectuyuni-repository

If you don't need a changelog check, please mark this checkbox:

No changelog needed

If you uncheck the checkbox after the PR is created, you will need to re-run changelog_test (see below)

Re-run a test

If you need to re-run a test, please mark the related checkbox, it will be unchecked automatically once it has re-run:

Re-run test "changelog_test"
Re-run test "backend_unittests_pgsql"
Re-run test "java_pgsql_tests"
Re-run test "schema_migration_test_pgsql"
Re-run test "susemanager_unittests"
Re-run test "javascript_lint"
Re-run test "spacecmd_unittests"

Before you merge

Check How to branch and merge properly!

agraul

I only looked at the Python code. I think there is a risk of an SQL injection, please check that again. I also left a few other comments. Many are nitpicks, feel free to ignore them.

python/spacewalk/satellite_tools/appstreams.py

agraul · 2024-04-12T11:13:11Z

python/spacewalk/satellite_tools/appstreams.py

+        sql = "INSERT INTO suseAppstreamApi (rpm, module_id) VALUES "
+        sql += ",".join([f"('{api}', {module_id})" for api in apis])
+        sql += " ON CONFLICT DO NOTHING"


Is this safe against SQL injections? I think we're handling untrusted input here.

python/spacewalk/satellite_tools/appstreams.py

agraul · 2024-04-12T11:18:59Z

python/spacewalk/satellite_tools/appstreams.py

+            # Remove the trailing ':' if epoch is present
+            epoch = match.group("epoch")[:-1] if match.group("epoch") else None


[nitpick] If you don't want to have to comment the code you can use rstrip instead. Either way is fine.

Suggested change

# Remove the trailing ':' if epoch is present

epoch = match.group("epoch")[:-1] if match.group("epoch") else None

epoch = match.group("epoch").rstrip(":") if match.group("epoch") else None

agraul · 2024-04-12T11:27:07Z

python/spacewalk/satellite_tools/reposync.py

+                                    modulemd_importer = ModuleMdImporter(
+                                        self.channel["id"], modulemd_path
+                                    )
+                                    modulemd_importer.validate()


This can raise an exception, can you add code that handles it? The general exception handler logs with "Unexpected error", maybe we can use a concrete logging statement instead.

agraul · 2024-04-12T11:29:00Z

python/spacewalk/satellite_tools/reposync.py

+            return self.copy_metadata_file(
+                plug, modulesfile, "modules", relative_modules_dir
+            )


Why was this changed?

Black insists on it (and all the others) 😞

Are you running black from the container? reposync.py was already reformatted with black, I wonder why yours has a different line length

agraul · 2024-04-12T11:29:13Z

python/spacewalk/satellite_tools/reposync.py

+                    to_disassociate[(db_pack["checksum_type"], db_pack["checksum"])] = (
+                        True
+                    )


Same here, why the reformatting?

agraul · 2024-04-12T11:31:35Z

python/test/unit/spacewalk/satellite_tools/test_appstreams.py

+
+        assert exc.value.domain == "modulemd-yaml-error-quark"
+
+    # pylint: disable=protected-access


[nitpick] This looks like something has the wrong visibility

This case tests the protected _get_modules method. Black doesn't let this without the disable directive.

python/spacewalk/satellite_tools/appstreams.py

agraul · 2024-04-12T11:42:46Z

python/test/unit/spacewalk/satellite_tools/test_appstreams.py

+# pylint: enable=wrong-import-position
+
+
+class TestModuleMdImporter:


[nitpick] There is no need for a class here.

cbbayburt force-pushed the appstreams-reposync branch from 3960103 to 72ebc8b Compare April 4, 2024 09:38

github-actions bot added schema_migration_test_pgsql database schema_migration_test_oracle labels Apr 4, 2024

cbbayburt force-pushed the appstreams-reposync branch 2 times, most recently from b1ba7a2 to 953e113 Compare April 5, 2024 11:40

cbbayburt requested a review from wweellddeerr April 5, 2024 11:41

cbbayburt marked this pull request as ready for review April 5, 2024 11:42

cbbayburt requested a review from a team as a code owner April 5, 2024 11:42

cbbayburt requested a review from vzhestkov April 5, 2024 11:42

cbbayburt marked this pull request as draft April 5, 2024 11:42

cbbayburt removed the request for review from vzhestkov April 5, 2024 11:42

cbbayburt changed the base branch from master to appstreams April 5, 2024 14:52

cbbayburt force-pushed the appstreams-reposync branch 3 times, most recently from 95f5c1b to c2a2862 Compare April 10, 2024 13:26

cbbayburt marked this pull request as ready for review April 10, 2024 16:45

cbbayburt requested review from a team and agraul and removed request for a team April 10, 2024 16:45

agraul requested changes Apr 12, 2024

View reviewed changes

wweellddeerr mentioned this pull request Apr 15, 2024

Appstreams UI #8585

Merged

18 tasks

cbbayburt force-pushed the appstreams-reposync branch 6 times, most recently from 33c4456 to 6762863 Compare April 16, 2024 15:19

cbbayburt requested a review from agraul April 16, 2024 15:23

m-czernek mentioned this pull request Apr 16, 2024

Keep Python linting container up to date #8607

Merged

1 task

Add DB tables for AppStreams module metadata

bb9635c

cbbayburt mentioned this pull request Apr 17, 2024

Adapt package queries for modular repositories #8525

Merged

12 tasks

agraul approved these changes Apr 18, 2024

View reviewed changes

cbbayburt force-pushed the appstreams-reposync branch from 97519ae to f4d314c Compare April 18, 2024 12:38

cbbayburt added 2 commits April 18, 2024 14:48

Add AppStreams reposync module

bbf44a3

Import module metadata in reposync

70c64c0

cbbayburt force-pushed the appstreams-reposync branch from f4d314c to 70c64c0 Compare April 18, 2024 12:49

cbbayburt merged commit 0a6faec into uyuni-project:appstreams Apr 18, 2024
8 of 9 checks passed

cbbayburt deleted the appstreams-reposync branch April 18, 2024 13:51

cbbayburt mentioned this pull request Apr 23, 2024

Native AppStreams support #8654

Merged

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

AppStreams reposync module #8421

AppStreams reposync module #8421

cbbayburt commented Mar 7, 2024 •

edited by galaxy-dashboard

Loading

agraul left a comment

agraul Apr 12, 2024

agraul Apr 12, 2024

agraul Apr 12, 2024

agraul Apr 12, 2024

cbbayburt Apr 12, 2024

agraul Apr 12, 2024

agraul Apr 12, 2024

agraul Apr 12, 2024

cbbayburt Apr 12, 2024

agraul Apr 12, 2024

		# Remove the trailing ':' if epoch is present
		epoch = match.group("epoch")[:-1] if match.group("epoch") else None

	# Remove the trailing ':' if epoch is present
	epoch = match.group("epoch")[:-1] if match.group("epoch") else None
	epoch = match.group("epoch").rstrip(":") if match.group("epoch") else None


		assert exc.value.domain == "modulemd-yaml-error-quark"

		# pylint: disable=protected-access

		# pylint: enable=wrong-import-position


		class TestModuleMdImporter:

AppStreams reposync module #8421

AppStreams reposync module #8421

Conversation

cbbayburt commented Mar 7, 2024 • edited by galaxy-dashboard Loading

Documentation

Test coverage

Links

Changelogs

Re-run a test

Before you merge

agraul 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cbbayburt commented Mar 7, 2024 •

edited by galaxy-dashboard

Loading