PR#201: Generate module artifacts rpms - copr/copr

copr / copr

#201 Generate module artifacts rpms

Merged 6 years ago by frostyx. Opened 6 years ago by frostyx.

copr/ frostyx/copr module-artifacts-rpms into master

[beaker-tests-sanity] improve the module install test

Jakub Kadlčík • 6 years ago

f2ec455

[frontend] set the gpg properties for the repo

Jakub Kadlčík • 6 years ago

b57e665

[backend] generate module artifacts rpms

Jakub Kadlčík • 6 years ago

a4187a8

backend/backend/actions.py

file modified

		`@@ -452,6 +452,12 @@`
		`self.log.info("Copy directory: {} as {}".format(`
		`os.path.join(srcdir, folder), os.path.join(destdir, folder)))`

		`+ for f in os.listdir(os.path.join(destdir, folder)):`
		`+ if not f.endswith(".rpm") or f.endswith(".src.rpm"):`
		`+ continue`
		`+ mmd.artifacts.rpms.add(str(f.rstrip(".rpm")))`
		`+`
		`+ self.log.info("Module artifacts: {}".format(mmd.artifacts.rpms))`
		`modulemd.dump_all(os.path.join(destdir, "modules.yaml"), [mmd])`
		`createrepo(path=destdir, front_url=self.front_url,`
		`username=ownername, projectname=projectname,`

beaker-tests/Sanity/copr-cli-basic-operations/runtest-modules.sh

file modified

+4 -1

		`@@ -200,7 +200,10 @@`

		rlAssertEquals "Module should be visible in the system" `dnf module list \|grep testmodule \|grep beakertest \|grep $DATE \|grep -v "Copr modules repo" \|wc -l` 1
		`rlRun "dnf module enable testmodule:beakertest"`
		`- rlRun "dnf module install testmodule/default"`
		`+ rlRun "dnf -y module install testmodule/default"`
		`+ rlRun "rpm -q mksh"`
		`+ rlRun "dnf -y module remove testmodule"`
		`+ rlRun "dnf -y module disable testmodule"`
		`rlRun "dnf -y downgrade dnf"`

frontend/coprs_frontend/coprs/templates/coprs/copr-modules.cfg

file modified

		`@@ -1,4 +1,7 @@`
		`[{{ copr.repo_id }}_{{ module.nsv }}]`
		`name = Copr modules repo for {{ module.full_name }}`
		`baseurl = {{ baseurl \| fix_url_https_backend }}`
		`+ gpgcheck={{ config.REPO_GPGCHECK \| default("1")}}`
		`+ gpgkey={{ pubkey_url \| fix_url_https_backend }}`
		`+ repo_gpgcheck=0`
		`enabled = 1`

frostyx commented 6 years ago

I've found out that it is possible to enable modules from Copr, see their info and so on, but it is not possible to install them. After some investigation with @mhatina we figured out, that we don't generate artifacts section [1] for our modules. So I've implemented it and I can confirm that it fixes the issue.

I needed the splitFilename function from frontend, so I've duplicated it to backend helpers. We really should make some python-copr-common package to share the code.

[1] https://pagure.io/modulemd/blob/master/f/spec.yaml#_256

clime commented 6 years ago

This doesn't work for Epoch:

clime@coprbox ~/rpkg-client $ rpm -qp --qf "%{NAME} %|EPOCH?{%{EPOCH}}:{(no Epoch)}|\n" ~/Downloads/rpkg-0.13-1.fc27.noarch.rpm
warning: /home/clime/Downloads/rpkg-0.13-1.fc27.noarch.rpm: Header V3 RSA/SHA1 Signature, key    ID de339fd2: NOKEY
rpkg 3

The rpm comes from this build here: http://copr-be-dev.cloud.fedoraproject.org/results/clime/projectx/fedora-27-x86_64/00651154-rpkg-client/

Also note that this function is not an argument to have python-copr-common because it should rather be placed in some package like python-rpmutils.

And finally note that "artifacts" is really a not-a-good name but that's just my side-note (it is a more fancy way to call some variable "things").

frostyx commented 6 years ago

Also note that this function is not an argument to have python-copr-common because it should rather be placed in some package like python-rpmutils.

There is a rpmUtils.miscutils.splitFilename. Why don't we use it?

And finally note that "artifacts" is really a not-a-good name but that's just my side-note (it is a more fancy way to call some variable "things").

Well, maybe ... but it isn't a name that I came up with. See the spec.yaml that I've linked. It is kind of an official name.

clime commented 6 years ago

Also note that this function is not an argument to have python-copr-common because it should rather be placed in some package like python-rpmutils.

There is a rpmUtils.miscutils.splitFilename. Why don't we use it?

And finally note that "artifacts" is really a not-a-good name but that's just my side-note (it is a more fancy way to call some variable "things").

Well, maybe ... but it isn't a name that I came up with. See the spec.yaml that I've linked. It is kind of an official name.

Yes, even in that spec.yaml, it is a bad name (or rather really a bad concept) but you are also using it for your own variable:

artifacts = set()

basically the same thing as:

things = set()

But I don't really care that much about this.

rpmUtils.miscutils.splitFilename

that comes from yum package, which is deprecated and it's also an overkill to install yum because of that function.

frostyx commented 6 years ago

This doesn't work for Epoch:

Ah, I see. That needs to be fixed. Thank you for pointing it out.

Yes, even in that spec.yaml, it is a bad name (or rather really a bad concept) but you are also using it for your own variable:

I will not argue with you about the inappropriate naming in spec.yaml. I probably don't have an opinion on it.

However, I think that that the

artifacts = set()

is quite clear name, considering that it's whole purpose is to gather some data that will be ultimately set to

mmd.artifacts.rpms = artifacts

I think that rpms would be confusing because I would expect rpms filenames in there. Another possibility would be artifacts_rpms which looks a bit cumbersome to me, but if you prefer it, or even other name, I will happily rename it.

clime commented 6 years ago

It seems that nevras would be a better name there (better than artifacts) but take that as a very optional suggestion.

4 new commits added

[frontend] set the gpg properties for the repo
[backend] refactor out the artifacts variable
[backend] don't parse the filename, just use it as is
[backend] dont use epoch, it is not needed

6 years ago

frostyx commented 6 years ago

It seems that nevras would be a better name there (better than artifacts) but take that as a very optional suggestion.

Good suggestion, haven't thought about that name. Anyway, I've refactored the code so the variable is not there anymore.

4 new commits added

It is written NEVRAs in the spec.yaml, but I've tested it and it works even with NVRAs. So I basically threw away the splitFilename code and used the whole RPM filename (without .rpm) instead.

There were also missing gpg properties in module repofile, so it failed on Error: GPG check FAILED when the checking was enabled.

Can you review again @clime, please? Btw, we should rebase the branch before merging and squash some of the commits.

clime commented 6 years ago

Looks ok.

rebased onto a4187a8

6 years ago

frostyx commented 6 years ago

I've rebased the branch and squashed some commits, but files are changed exactly the same. I've also improved the tests, so it now checks whether the packages provided by the module are installed.

I think I can merge it.

Pull-Request has been merged by frostyx

6 years ago