libtaskotron/directives/mash_directive.py
86–87	This is the old documentation, please pull from `develop` and rebase it. Then adjust the `outdir` documentation to state that the dir will be created automatically if not present.
171	I'd put something like # create outdir if it doesn't exist above this line. It's much easier to skim the code then.
172–176	I don't understand why this code is needed. The `python_utils.makedirs()` function is a modified `os.makedirs()` to work like `mkdir -p` in bash (except the parent creation). I think the code in the `else` block should be fully sufficient - the error will be thrown only if the dir can't be created, but not if it is already present. Maybe it's better to explicitly call `python_utils.makedirs()` instead of `makedirs()` so that it's clear this is our modified version and prior checks for dir existence are not required?

libtaskotron/directives/mash_directive.py
37–38	If I can nitpick, I'd write it as ``outdir``
86–87	Ouch, by some mistake this docstring was not deleted when `DOCUMENTATION` and `EXAMPLES` were added. Please remove this whole docstring, thanks.
testing/functest_mash_directive.py
9–10	I find a bit confusing to import something named `and_`, and to a lesser extent even `SimpleRpmBuild`. For non-descriptive names, or for less frequent modules, please consider importing the whole module and then using e.g. `operator.and_`. The code will get more readable. Of course, if you need to use it on every second line, it makes sense to try to have the name shorter. If you need to use it just once or twice, however... Btw, some of these names don't seem to be used at all, says pyflakes.
75	Instead of using `tempfile` module, could you please rather use `tmpdir` fixture? Not just here, in the whole module. https://pytest.org/latest/tmpdir.html It has many advantages over standard `tempfile` module. We can easily control where the files are created and the files/dirs are kept there only for a limited time, then they're deleted by pytest. Also, it's very easy to find these files and debug the unit test when something goes wrong. Have a look at other functional tests how to use `tmpdir` fixture.
87	According to documentation it seems that `isdir()` implies `exists()`.

Diff	ID	Base	Description	Created	Lint	Unit
Base			Base
Diff 1	486	63c2ebf		Jun 27 2014, 10:32 AM	★	★
Diff 2	488	63c2ebf	- polishing	Jun 27 2014, 10:42 AM	★	★
Diff 3	498	8fc611c	- polishing 2	Jun 30 2014, 11:55 AM	★	★
Diff 4	500	8fc611c	- polishing 3	Jul 1 2014, 8:42 AM	★	★
Diff 5	501	50dd5f8	Commit rLTRN2294b06707e7b26d993f20878b405599d898373d	Jul 1 2014, 10:47 AM	★	★

Commit	Tree	Parents	Author	Summary	Date
5e1e44a5b924	cc39e57ab94e	efd11ff56e29	Lukas Brabec	polishing 3	Jul 1 2014, 8:42 AM
efd11ff56e29	c061ef8bea18	353af6473960	Lukas Brabec	Polishing 2	Jun 30 2014, 11:52 AM
353af6473960	ebe8c8ef8b71	7767e99a0f08	Lukas Brabec	polishing	Jun 27 2014, 10:39 AM
7767e99a0f08	e83d26e5ecde	8fc611c82b09	Lukas Brabec	mash_directive: check outdir existence, creation (Show More…)	Jun 27 2014, 10:28 AM

Diff 501

libtaskotron/directives/mash_directive.py

Show All 28 Lines		23	parameters:
29	arch:	29		arch:
30	required: true	30		required: true
31	description: list of architectures for which to create YUM metadata	31		description: list of architectures for which to create YUM metadata
32	type: list of str	32		type: list of str
33	outdir:	33		outdir:
34	required: false	34		required: false
35	description: absolute or relative path to an output directory, where	35		description: absolute or relative path to an output directory, where
36	repository should be created. If not specified, then ``rpmdir`` is used	36		repository should be created. If not specified, then ``rpmdir`` is used
37	as an output directory.	37		as an output directory. ``outdir`` will be created automatically if not
		38		present.
			kparalUnsubmitted Not Done If I can nitpick, I'd write it as ``outdir``
38	type: str	39		type: str
39	default: None	40		default: None
40	dodelta:	41		dodelta:
41	required: false	42		required: false
42	description: whether to create delta RPMs during the mash process	43		description: whether to create delta RPMs during the mash process
43	type: bool	44		type: bool
44	default: False	45		default: False
45	choices: [True, False]	46		choices: [True, False]
Show All 28 Lines
74	import mash	75		import mash
75	import mash.config	76		import mash.config
76	import StringIO	77		import StringIO
77	from ConfigParser import RawConfigParser	78		from ConfigParser import RawConfigParser
78		79
79	from libtaskotron.directives import BaseDirective	80		from libtaskotron.directives import BaseDirective
80	from libtaskotron.logger import log	81		from libtaskotron.logger import log
81	from libtaskotron.exceptions import TaskotronDirectiveError	82		from libtaskotron.exceptions import TaskotronDirectiveError
		83		import libtaskotron.file_utils as file_utils
82		84
83	directive_class = 'MashDirective'	85		directive_class = 'MashDirective'
84		86
85	class MashDirective(BaseDirective):
86	"""This will create a YUM repository in ``outdir`` from from all RPM
87	packages in ``rpmdir``. If ``outdir`` is not specified, repo is created
88	in ``rpmdir``. It will call ``mash`` tool to perform this task.
89
90	The directive returns actual path where repo was created.
91
92	For the mash directive, we're expecting a yaml declaration of the form:
93
94	mash:
95	rpmdir=/path/to/some/dir
96	dodelta=[True, False]
97	arch=["i386", "x86_64", "armhfp", "noarch"]
98	"""
99		87
			kparalUnsubmitted Not Done This is the old documentation, please pull from `develop` and rebase it. Then adjust the `outdir` documentation to state that the dir will be created automatically if not present.
			kparalUnsubmitted Not Done Ouch, by some mistake this docstring was not deleted when `DOCUMENTATION` and `EXAMPLES` were added. Please remove this whole docstring, thanks.
		88		class MashDirective(BaseDirective):
100	def do_mash(self, rpmdir, dodelta, arch, outdir=None):	89		def do_mash(self, rpmdir, dodelta, arch, outdir=None):
101	"""Set up a mash object with an ad-hoc config	90		"""Set up a mash object with an ad-hoc config
102		91
103	:param str rpmdir: directory containing rpms to mash	92		:param str rpmdir: directory containing rpms to mash
104	:param bool dodelta: create drpms during mash	93		:param bool dodelta: create drpms during mash
105	:param str arch: arch of the rpms	94		:param str arch: arch of the rpms
106	:param str outdir: output directory where repo is created,	95		:param str outdir: output directory where repo is created,
107	if not specified or None, rpmdir is used	96		if not specified or None, ``rpmdir`` is used.
		97		``outdir`` will be created automatically if
		98		not present.
108	:rtype str	99		:rtype str
109	:returns: path to directory where repo was created	100		:returns: path to directory where repo was created
110	"""	101		"""
111		102
112	# copied over from the old depcheck	103		# copied over from the old depcheck
113	distname = 'depcheck'	104		distname = 'depcheck'
114	mash_conf_items = ["[%s]" % distname,	105		mash_conf_items = ["[%s]" % distname,
115	"output_dir = ",	106		"output_dir = ",
Show All 26 Lines
142		133
143	log.info('running createrepo on %s', rpmdir)	134		log.info('running createrepo on %s', rpmdir)
144	if outdir:	135		if outdir:
145	p = sub.Popen(['createrepo', '-o', outdir, rpmdir], stdout=sub.PIPE,	136		p = sub.Popen(['createrepo', '-o', outdir, rpmdir], stdout=sub.PIPE,
146	stderr=sub.PIPE)	137		stderr=sub.PIPE)
147	for rpm in os.listdir(rpmdir):	138		for rpm in os.listdir(rpmdir):
148	os.symlink(os.path.join(rpmdir, rpm), os.path.join(outdir, rpm))	139		os.symlink(os.path.join(rpmdir, rpm), os.path.join(outdir, rpm))
149	else:	140		else:
150	p = sub.Popen(['createrepo', rpmdir], stdout=sub.PIPE, stderr=sub.PIPE)	141		p = sub.Popen(['createrepo', rpmdir], stdout=sub.PIPE,
		142		stderr=sub.PIPE)
151		143
152	output, errors = p.communicate()	144		output, errors = p.communicate()
153		145
154	if p.returncode:	146		if p.returncode:
155	raise TaskotronDirectiveError(errors)	147		raise TaskotronDirectiveError(errors)
156		148
157	log.info('running mash multilib on %s', rpmdir)	149		log.info('running mash multilib on %s', rpmdir)
158	themash.doMultilib()	150		themash.doMultilib()
Show All 12 Lines		162	if 'dodelta' not in input_data:
171	dodelta = False	163		dodelta = False
172	else:	164		else:
173	dodelta = input_data['dodelta']	165		dodelta = input_data['dodelta']
174		166
175	rpmdir = input_data['rpmdir']	167		rpmdir = input_data['rpmdir']
176	outdir = input_data.get('outdir', None)	168		outdir = input_data.get('outdir', None)
177	arch = input_data['arch']	169		arch = input_data['arch']
178		170
		171		# create outdir if it doesn't exist
			kparalUnsubmitted Not Done I'd put something like # create outdir if it doesn't exist above this line. It's much easier to skim the code then.
		172		if outdir:
		173		try:
		174		file_utils.makedirs(outdir)
		175		except OSError, e:
		176		log.exception("Can't create directory: %s", outdir)
			kparalUnsubmitted Not Done I don't understand why this code is needed. The `python_utils.makedirs()` function is a modified `os.makedirs()` to work like `mkdir -p` in bash (except the parent creation). I think the code in the `else` block should be fully sufficient - the error will be thrown only if the dir can't be created, but not if it is already present. Maybe it's better to explicitly call `python_utils.makedirs()` instead of `makedirs()` so that it's clear this is our modified version and prior checks for dir existence are not required?
		177		raise TaskotronDirectiveError(e)
		178
179	return self.do_mash(rpmdir, dodelta, arch, outdir)	179		return self.do_mash(rpmdir, dodelta, arch, outdir)

View Options

testing/functest_mash_directive.py

1	# -- coding: utf-8 --	1		# -- coding: utf-8 --
2	# Copyright 2009-2014, Red Hat, Inc.	2		# Copyright 2009-2014, Red Hat, Inc.
3	# License: GPL-2.0+ <http://spdx.org/licenses/GPL-2.0+>	3		# License: GPL-2.0+ <http://spdx.org/licenses/GPL-2.0+>
4	# See the LICENSE file for more details on Licensing	4		# See the LICENSE file for more details on Licensing
5		5
6	from libtaskotron.directives import mash_directive	6		from libtaskotron.directives import mash_directive
7	from libtaskotron.exceptions import TaskotronDirectiveError
8	import os	7		import os
9	import shutil	8		import shutil
10	import pytest	9		import pytest
11	from tempfile import mkdtemp	10		import rpmfluff
			kparalUnsubmitted Not Done I find a bit confusing to import something named `and_`, and to a lesser extent even `SimpleRpmBuild`. For non-descriptive names, or for less frequent modules, please consider importing the whole module and then using e.g. `operator.and_`. The code will get more readable. Of course, if you need to use it on every second line, it makes sense to try to have the name shorter. If you need to use it just once or twice, however... Btw, some of these names don't seem to be used at all, says pyflakes.
12	from rpmfluff import SimpleRpmBuild
13	from operator import and_
14		11
15		12
16	class TestMashDirective(object):	13		class TestMashDirective(object):
17	def setup_method(self, method):	14		@pytest.fixture(autouse=True)
		15		def setup_method(self, tmpdir):
18	NVRAs = [	16		NVRAs = [
19	["wine", "0.1", "3", ["i686", "x86_64"]],	17		["wine", "0.1", "3", ["i686", "x86_64"]],
20	["foo", "0.2", "2", ["i686"]],	18		["foo", "0.2", "2", ["i686"]],
21	]	19		]
22		20
23	#store current dir	21		#store current dir
24	startdir = os.getcwd()	22		startdir = os.getcwd()
25		23
26	workdir = mkdtemp()	24		workdir = tmpdir.strpath
27	self.rpmdir = os.path.join(workdir, "rpmdir")	25		self.rpmdir = os.path.join(workdir, "rpmdir")
28		26
29	print self.rpmdir	27		print self.rpmdir
30		28
31	os.chdir(workdir)	29		os.chdir(workdir)
32	os.mkdir(self.rpmdir)	30		os.mkdir(self.rpmdir)
33		31
34	packages = [SimpleRpmBuild(*NVRA) for NVRA in NVRAs]	32		packages = [rpmfluff.SimpleRpmBuild(*NVRA) for NVRA in NVRAs]
35		33
36	for package in packages:	34		for package in packages:
37	package.make()	35		package.make()
38		36
39	rpms = [p.get_built_rpm("i686") for p in packages]	37		rpms = [p.get_built_rpm("i686") for p in packages]
40		38
41	for rpm in rpms:	39		for rpm in rpms:
42	shutil.copy(rpm, self.rpmdir)	40		shutil.copy(rpm, self.rpmdir)
Show All 25 Lines		61	def test_mash_arch_x86_64(self):
68	directive = mash_directive.MashDirective()	66		directive = mash_directive.MashDirective()
69	before = os.listdir(self.rpmdir)	67		before = os.listdir(self.rpmdir)
70	directive.process(input_data, {})	68		directive.process(input_data, {})
71	after = os.listdir(self.rpmdir)	69		after = os.listdir(self.rpmdir)
72		70
73	assert self.rpms[0] in before and self.rpms[0] in after and \	71		assert self.rpms[0] in before and self.rpms[0] in after and \
74	self.rpms[1] in before and self.rpms[1] not in after	72		self.rpms[1] in before and self.rpms[1] not in after
75		73
		74		def test_outdir_is_created(self, tmpdir):
		75		outdir_path = tmpdir.join('somewhere/along/the/highway').strpath
			kparalUnsubmitted Not Done Instead of using `tempfile` module, could you please rather use `tmpdir` fixture? Not just here, in the whole module. https://pytest.org/latest/tmpdir.html It has many advantages over standard `tempfile` module. We can easily control where the files are created and the files/dirs are kept there only for a limited time, then they're deleted by pytest. Also, it's very easy to find these files and debug the unit test when something goes wrong. Have a look at other functional tests how to use `tmpdir` fixture.
		76		input_data = {
		77		'rpmdir': self.rpmdir,
		78		'outdir': outdir_path,
		79		'arch': ['i386']
		80		}
		81
		82		directive = mash_directive.MashDirective()
		83		ret_path = directive.process(input_data, {})
		84
		85		assert ret_path == outdir_path
		86		assert os.path.isdir(ret_path)
		87
			kparalUnsubmitted Not Done According to documentation it seems that `isdir()` implies `exists()`.
		88
76	def test_repodata_in_rpmdir(self):	89		def test_repodata_in_rpmdir(self):
77	input_data = {	90		input_data = {
78	'rpmdir': self.rpmdir,	91		'rpmdir': self.rpmdir,
79	'arch': ['x86_64']	92		'arch': ['x86_64']
80	}	93		}
81	directive = mash_directive.MashDirective()	94		directive = mash_directive.MashDirective()
82	directive.process(input_data, {})	95		directive.process(input_data, {})
83	after = os.listdir(self.rpmdir)	96		after = os.listdir(self.rpmdir)
84		97
85	assert 'repodata' in after	98		assert 'repodata' in after
86		99
87	def test_repodata_in_outdir(self):	100		def test_repodata_in_outdir(self, tmpdir):
88	outdir = mkdtemp()	101		outdir = tmpdir.strpath
89	input_data = {	102		input_data = {
90	'rpmdir': self.rpmdir,	103		'rpmdir': self.rpmdir,
91	'outdir': outdir,	104		'outdir': outdir,
92	'arch': ['x86_64']	105		'arch': ['x86_64']
93	}	106		}
94	directive = mash_directive.MashDirective()	107		directive = mash_directive.MashDirective()
95	directive.process(input_data, {})	108		directive.process(input_data, {})
96	after = os.listdir(self.rpmdir)	109		after = os.listdir(self.rpmdir)
97		110
98	assert 'repodata' not in after and 'repodata' in os.listdir(outdir)	111		assert 'repodata' not in after and 'repodata' in os.listdir(outdir)
99

			Path	Packages
M			libtaskotron/directives/mash_directive.py (34 lines)
M			testing/functest_mash_directive.py (32 lines)

mash_directive: check outdir existence, creation
ClosedPublic
Actions

Details

Diff Detail

Revision Contents

Diff 501

libtaskotron/directives/mash_directive.py

testing/functest_mash_directive.py

mash_directive: check outdir existence, creationClosedPublicActions

Details

Diff Detail

Revision Contents

Diff 501

libtaskotron/directives/mash_directive.py

testing/functest_mash_directive.py

mash_directive: check outdir existence, creation
ClosedPublic
Actions