Add new command modulesync #444

j-mracek · 2021-10-08T07:08:05Z

It will download module metadata from all enabled repositories,
module artifacts and profiles of matching modules. Then it creates
a repository.

Requires: rpm-software-management/dnf#1794
For RHEL8 it requires: rpm-software-management/libdnf#1381

pep8speaks · 2021-11-02T13:38:38Z

Hello @j-mracek! 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 2021-12-08 13:45:36 UTC

m-blaha · 2021-11-09T07:56:25Z

I'm not sure how useful is tracking the tool development in separate commits. I think squashing all of them into one commit which introduces the new plugin is cleaner.
But feel free to ignore this rant, it isn't that important.

dnf-plugins-core.spec

m-blaha · 2021-11-09T09:41:59Z

doc/modulesync.rst

+
+`modulesync` downloads packages from modules according to provided arguments and creates a repository with modular data
+in working directory. Im modular environment it is recommend to use the command in combination with other dnf commands
+which downloads RPM packages. See examples


I'm a bit confused here. Could you elaborate in more details the use case when user needs to download rpm packages separately (using dnf download or such) and then run modulesync to generate modular metadata? I thought that modulesync will download rpm packages from given modules.

In case that a package has modular label in rpm header it cannot be used in new repository without modular metadata. module sync will create a repository including modular metadata.

But modulesync also downloads the rpm package itself, doesn't it? What I do not understand is the need of dnf download before dnf modulesync. I really think this deserves more detailed documentation in the man pages.

j-mracek · 2021-11-10T13:36:10Z

I'm not sure how useful is tracking the tool development in separate commits. I think squashing all of them into one commit which introduces the new plugin is cleaner. But feel free to ignore this rant, it isn't that important.

I can do it.

doc/modulesync.rst

inknos · 2021-11-16T14:31:31Z

Please, could you update the commit message with =changelog= and specify the msg, type and bug fileds?

m-blaha · 2021-12-08T12:25:45Z

doc/modulesync.rst

+# Copyright (C) 2021  Red Hat, Inc.
+#
+# This copyrighted material is made available to anyone wishing to use,
+# modify, copy, or redistribute it subject to the terms and conditions of
+# the GNU General Public License v.2, or (at your option) any later version.
+# This program is distributed in the hope that it will be useful, but WITHOUT
+# ANY WARRANTY expressed or implied, including the implied warranties of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General
+# Public License for more details.  You should have received a copy of the
+# GNU General Public License along with this program; if not, write to the
+# Free Software Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA
+# 02110-1301, USA.  Any Red Hat trademarks that are incorporated in the
+# source code or documentation are not subject to the GNU General Public
+# License and may only be used or replicated with the express permission of
+# Red Hat, Inc.
+#
+


Copyright here is not correctly formated - resulting in inclusion of copyright into the man page itself. Correct format of the comment in rst file is:

.. Copyright (C) 2021 Red Hat, Inc. This copyrighted...

m-blaha · 2021-12-08T12:27:16Z

doc/modulesync.rst

+-----------
+
+`modulesync` downloads packages from modules according to provided arguments and creates a repository with modular data
+in working directory. Im environment with modules it is recommend to use the command for redistribution of packages,


typo: In environment

m-blaha · 2021-12-08T12:31:53Z

doc/modulesync.rst

+
+`modulesync` downloads packages from modules according to provided arguments and creates a repository with modular data
+in working directory. Im environment with modules it is recommend to use the command for redistribution of packages,
+because DNF prevents from getting modular packages without modular metadata on the system (Fail-safe mechanism).


because DNF does not allow installation of modular packages without modular metadata...?
I'm not sure what getting word here is supposed to mean.

m-blaha · 2021-12-08T12:35:27Z

doc/modulesync.rst

+``dnf download nodejs``
+``dnf modulesync``


Multiline examples are incorrectly formatted. In the man page they are rendered as one line e.g.:

dnf download nodejs dnf modulesync The first download command ...

m-blaha · 2021-12-08T12:40:10Z

doc/modulesync.rst

+``dnf --destdir=/tmp/my-temp modulesync nodejs:14/minimal --resolve``
+    Download package required for installation of `minimal` profile from module `nodejs` and stream `14` into directory
+    `/tmp/my-temp` and all required dependencies. Then it will create a repository in `/tmp/my-temp` directory with
+    packages previously downloaded packages and modular metadata from all available repositories.


duplicated packages word

m-blaha · 2021-12-08T12:40:44Z

doc/modulesync.rst

+``dnf modulesync --destdir=/tmp/my-temp``
+    The first `dnf module install` command downloads package from required for installation of `minimal` profile from module
+    `nodejs` and stream `14` into directory `/tmp/my-temp`. The second command `dnf modulesync` will create
+    a repository in `/tmp/my-temp` directory with packages previously downloaded packages and modular metadata from all


Again duplicated packages word.

m-blaha

I found some remaining issues with man pages / documentation. Also the modulesync should be included in index.rst file.

m-blaha · 2021-12-08T13:22:23Z

plugins/modulesync.py

+
+        return result_query
+
+    def _get_providers_of_requires(self, to_test, done=None, req_dict={}):


using mutable default parameter values (here req_dict={}) is strongly discouraged and has significant side effects (see https://florimond.dev/en/posts/2018/08/python-mutable-defaults-are-the-source-of-all-evil/). Unless you have a reason, please use req_dict=None.

Good point.

It will download module metadata from all enabled repositories, module artifacts and profiles of matching modules. Then it creates a repository. = changelog = msg: Add a new subpackage with modulesync command. The command downloads packages from modules and/or creates a repository with modular data. type: enhancement resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1868047

j-mracek added ready for review test required labels Nov 2, 2021

j-mracek force-pushed the sync-tool branch from 50c7387 to 9dbeeda Compare November 2, 2021 13:44

j-mracek marked this pull request as ready for review November 2, 2021 13:45

j-mracek force-pushed the sync-tool branch from 9dbeeda to 6b84d03 Compare November 2, 2021 13:47

m-blaha self-assigned this Nov 9, 2021

m-blaha reviewed Nov 9, 2021

View reviewed changes

inknos self-assigned this Nov 9, 2021

j-mracek force-pushed the sync-tool branch 2 times, most recently from 865a7eb to 4523ed0 Compare November 11, 2021 14:53

inknos requested changes Nov 15, 2021

View reviewed changes

doc/modulesync.rst Outdated Show resolved Hide resolved

doc/modulesync.rst Outdated Show resolved Hide resolved

doc/modulesync.rst Outdated Show resolved Hide resolved

inknos mentioned this pull request Nov 16, 2021

Add tests for modulesync rpm-software-management/ci-dnf-stack#1055

Merged

j-mracek force-pushed the sync-tool branch 2 times, most recently from 97eedc1 to fd2f1ca Compare November 23, 2021 14:11

m-blaha reviewed Dec 8, 2021

View reviewed changes

m-blaha requested changes Dec 8, 2021

View reviewed changes

j-mracek force-pushed the sync-tool branch 2 times, most recently from af84b0d to faab2fe Compare December 8, 2021 12:57

m-blaha reviewed Dec 8, 2021

View reviewed changes

j-mracek force-pushed the sync-tool branch from faab2fe to a2ad648 Compare December 8, 2021 13:45

m-blaha merged commit 4c31b66 into rpm-software-management:master Dec 8, 2021

Add new command modulesync #444

Add new command modulesync #444

j-mracek commented Oct 8, 2021 •

edited

pep8speaks commented Nov 2, 2021 •

edited

m-blaha commented Nov 9, 2021

m-blaha Nov 9, 2021

j-mracek Nov 10, 2021

m-blaha Nov 11, 2021

j-mracek Nov 11, 2021

j-mracek commented Nov 10, 2021

inknos commented Nov 16, 2021

m-blaha Dec 8, 2021

m-blaha Dec 8, 2021

m-blaha Dec 8, 2021

m-blaha Dec 8, 2021 •

edited

m-blaha Dec 8, 2021

m-blaha Dec 8, 2021

m-blaha left a comment

m-blaha Dec 8, 2021

j-mracek Dec 8, 2021

j-mracek Dec 8, 2021


		return result_query

		def _get_providers_of_requires(self, to_test, done=None, req_dict={}):

Add new command modulesync #444

Add new command modulesync #444

Conversation

j-mracek commented Oct 8, 2021 • edited

pep8speaks commented Nov 2, 2021 • edited

Comment last updated at 2021-12-08 13:45:36 UTC

m-blaha commented Nov 9, 2021

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

j-mracek commented Nov 10, 2021

inknos commented Nov 16, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

m-blaha Dec 8, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

m-blaha 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

j-mracek commented Oct 8, 2021 •

edited

pep8speaks commented Nov 2, 2021 •

edited

m-blaha Dec 8, 2021 •

edited