Differential D1069

Improvements to the dist-git directive.
ClosedPublic
Actions

Authored by • ralph2 on Dec 9 2016, 1:41 PM.

Details

Reviewers
kparal
tflink
mkrizek
jskladan
Commits
rLTRNc63217671804: Improvements to the dist-git directive.
rLTRNd6ffc6720255: Merge branch 'distgit-improvements' into develop
rLTRNa37cc672f423: Allow dist_git_commit as a possible item_type.
Summary

Our use case is that we want to be able to run the modlint tool in
response to dist-git commits to modulemd.yaml files. modlint is kind
of like rpmlint, except for module definitions.

We want to run those checks in response to dist-git commits via
taskotron-trigger.

We were excited to see that a dist-git directive already exists, but it
assumed that you were specifying your request in terms of an rpm NEVR
which we do not have in our use case.

This changes opens it up to be able to pull down any file from any
namespace of dist-git repo, on any branch. It is up to the caller of
the dist-git directive to specify all of that information.

Also, allow dist_git_commit as a possible item_type. This is for
https://github.com/mmathesius/check_modulemd/pull/1

Also, I originally filed this as https://phab.qadevel.cloud.fedoraproject.org/D1055,
but I had issues updating the reviewers, so I've recreated it here.

Test Plan
Run the test suite.

Diff Detail

Repository
rLTRN libtaskotron
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
• ralph2 retitled this revision from to Improvements to the dist-git directive..Dec 9 2016, 1:41 PM
• ralph2 updated this object.
• ralph2 edited the test plan for this revision. (Show Details)
• ralph2 added reviewers: tflink, jskladan, mkrizek, kparal.
jskladan accepted this revision.Dec 12 2016, 5:36 AM

Good work, thanks you! Sorry for taking so long to review...

This revision is now accepted and ready to land.Dec 12 2016, 5:36 AM
Closed by commit rLTRNc63217671804: Improvements to the dist-git directive. (authored by Ralph Bean <rbean@redhat.com>). · Explain WhyDec 12 2016, 2:52 PM
This revision was automatically updated to reflect the committed changes.
kparal added a comment.Jan 23 2017, 1:16 PM

I have one concern about this, with the current change this makes it harder to use from koji_build tasks (where you receive NEVR as item). Before this you could have simply provided it as an argument. Now you need to parse it, and you need to do that manually in an external script, similarly to what you do for check_modulemd:

- name: split the given commit identifier into useful pieces
  python:
    file: split_string.py
    callable: run
    string: ${dist_git_commit}
  export: c

The best approach would be if we provided structured input data from the start (so that koji_build would be immediately also available as ${koji_build.name}, ${koji_build.version} etc). That might be a bigger change, so I'll probably provide a quick compatibility patch for distgit directive in the meantime and add a new option to accept NEVR, so that this is still easy to use for distgit rpm tasks. Thoughts?

Revision Contents

Diff 2721

View Options

libtaskotron/directives/distgit_directive.py

Show All 10 Lines
11description: | 11​description: |
12 Download files from Fedora package repository (usually called 'distgit') hosted at 12​ Download files from Fedora package repository (usually called 'distgit') hosted at
13 http://pkgs.fedoraproject.org/. Any files hosted at that repository can be downloaded for a 13​ http://pkgs.fedoraproject.org/. Any files hosted at that repository can be downloaded for a
14 chosen package. 14​ chosen package.
15parameters: 15​parameters:
16 package: 16​ package:
17 required: true 17​ required: true
18 description: | 18​ description: |
19 N(E)VR of a package. Package dist tag determines the git branch used for file download. 19​ Name of a package.
20 Example: ``xchat-2.8.8-21.fc20`` 20​ Example: ``xchat``
21 type: str 21​ type: str
22​ gitref:
23​ required: false
24​ description: |
25​ A git ref to check out. May be a branch, tag, or commit hash.
26​ Example: ``f24`` or ``04164165a840405e6bb5acc54a51e22346d84e0d``
27​ type: str
28​ default: master
29​ namespace:
30​ required: false
31​ description: |
32​ dist-git namespace to use when constructing the url to clone.
33​ Example: ``modules``, ``rpms`` or ``docker``
34​ type: str
35​ default: rpms
22 path: 36​ path:
23 required: true 37​ required: true
24 description: | 38​ description: |
25 files (directories not supported at the moment) to be downloaded from distgit. 39​ files (directories not supported at the moment) to be downloaded from distgit.
26 Example: ``[xchat.spec]`` 40​ Example: ``[xchat.spec]``
27 type: list of str 41​ type: list of str
28 localpath: 42​ localpath:
29 required: false 43​ required: false
30 description: | 44​ description: |
31 a local path of downloaded file. If not provided, path from distgit will be used. 45​ a local path of downloaded file. If not provided, path from distgit will be used.
32 Example: ``[specs/xchat.spec]`` 46​ Example: ``[specs/xchat.spec]``
33 type: list of str 47​ type: list of str
34 target_dir: 48​ target_dir:
35 required: false 49​ required: false
36 description: directory into which to download files 50​ description: directory into which to download files
37 type: str 51​ type: str
38 default: ${workdir} 52​ default: ${workdir}
53​ baseurl:
54​ required: false
55​ description: |
56​ The baseurl to use for dist-git. Defaults to Fedora's production instance.
57​ Example: ``http://pkgs.stg.fedoraproject.org``
58​ type: str
59​ default: http://pkgs.fedoraproject.org
39returns: | 60​returns: |
40 A dictionary containing following items: 61​ A dictionary containing following items:
41 62​
42 * `downloaded_files`: (list of str) a list of local filenames of the downloaded 63​ * `downloaded_files`: (list of str) a list of local filenames of the downloaded
43 files 64​ files
44raises: | 65​raises: |
45 * :class:`.TaskotronRemoteError`: if downloading failed 66​ * :class:`.TaskotronRemoteError`: if downloading failed
46 * :class:`.TaskotronValueError`: if path and localpath are not lists or are not of the same 67​ * :class:`.TaskotronValueError`: if path and localpath are not lists or are not of the same
47 length 68​ length
48 * :class:`.TaskotronDirectiveError`: if package or path is missing 69​ * :class:`.TaskotronDirectiveError`: if package or path is missing
49version_added: 0.3.16 70​version_added: 0.3.16
50""" 71​"""
51 72​
52EXAMPLES = """ 73​EXAMPLES = """
53A task needs to download a spec file and httpd configuration check to run a check 74​A task needs to download a spec file and httpd configuration check to run a check
54on those files:: 75​on those files::
55 76​
56 - name: download spec file and httpd conf 77​ - name: download spec file and httpd conf
57 distgit: 78​ distgit:
58 package: yourls-1.7-3.20150410gitabc7d6c.fc22 79​ package: yourls
80​ gitref: f22
59 path: 81​ path:
60 - yourls.spec 82​ - yourls.spec
61 - yourls-httpd.conf 83​ - yourls-httpd.conf
62 localpath: 84​ localpath:
63 - download/yourls_downloaded.spec 85​ - download/yourls_downloaded.spec
64 - download/yourls-httpd_downloaded.conf 86​ - download/yourls-httpd_downloaded.conf
65""" 87​"""
66 88​
67import os.path 89​import os.path
68 90​
69from libtaskotron.directives import BaseDirective 91​from libtaskotron.directives import BaseDirective
70from libtaskotron import file_utils, python_utils 92​from libtaskotron import file_utils, python_utils
71from libtaskotron.ext.fedora import rpm_utils ​
72 93​
73import libtaskotron.exceptions as exc 94​import libtaskotron.exceptions as exc
74 95​
75 96​
76 97​
77directive_class = 'DistGitDirective' 98​directive_class = 'DistGitDirective'
78 99​
79URL_FMT = 'http://pkgs.fedoraproject.org/cgit/%s.git/plain/%s?h=%s' 100​URL_FMT = '{baseurl}/cgit/{namespace}/{package}.git/plain/{path}?h={gitref}'
80 101​
81 102​
82class DistGitDirective(BaseDirective): 103​class DistGitDirective(BaseDirective):
83 def __init__(self): 104​ def __init__(self):
84 super(DistGitDirective, self).__init__() 105​ super(DistGitDirective, self).__init__()
85 106​
86 def _download_file(self, package, path, localpath): ​
87 pkgname = rpm_utils.rpmformat(package, fmt='n') ​
88 dist_tag = rpm_utils.get_dist_tag(package) ​
89 # fc22 -> f22 conversion, since that's how branches are called ​
90 branch = dist_tag.replace('c', '') ​
91 url = URL_FMT % (pkgname, path, branch) ​
92 ​
93 return file_utils.download(url, '.', localpath) ​
94 ​
95 def process(self, params, arg_data): 107​ def process(self, params, arg_data):
96 if 'package' not in params or 'path' not in params: 108​ if 'package' not in params or 'path' not in params:
97 detected_args = ', '.join(params.keys()) 109​ detected_args = ', '.join(params.keys())
98 raise exc.TaskotronDirectiveError( 110​ raise exc.TaskotronDirectiveError(
99 "The distgit directive requires 'package' and 'path' arguments." 111​ "The distgit directive requires 'package' and 'path' arguments."
100 "Detected arguments: %s" % detected_args) 112​ "Detected arguments: %s" % detected_args)
101 113​
102 if 'target_dir' not in params: 114​ # Assign defaults
103 target_dir = arg_data['workdir'] 115​ gitref = params.get('gitref', 'master')
104 else: 116​ namespace = params.get('namespace', 'rpms')
105 target_dir = params['target_dir'] 117​ baseurl = params.get('baseurl', 'http://pkgs.fedoraproject.org')
118​ target_dir = params.get('target_dir', arg_data['workdir'])
119​
106 120​
107 if not python_utils.iterable(params['path']): 121​ if not python_utils.iterable(params['path']):
108 raise exc.TaskotronValueError("Incorrect value type of the 'path' argument: " 122​ raise exc.TaskotronValueError("Incorrect value type of the 'path' argument: "
109 "%s" % type(params['path'])) 123​ "%s" % type(params['path']))
110 124​
111 target_path = params['path'] 125​ target_path = params['path']
112 output_data = {} 126​ output_data = {}
113 127​
114 if 'localpath' in params: 128​ if 'localpath' in params:
115 if not python_utils.iterable(params['localpath']): 129​ if not python_utils.iterable(params['localpath']):
116 raise exc.TaskotronValueError("Incorrect value type of the 'localpath' argument: " 130​ raise exc.TaskotronValueError("Incorrect value type of the 'localpath' argument: "
117 "%s" % type(params['path'])) 131​ "%s" % type(params['path']))
118 132​
119 if not len(params['path']) == len(params['localpath']): 133​ if not len(params['path']) == len(params['localpath']):
120 raise exc.TaskotronValueError('path and localpath lists must be of the same ' 134​ raise exc.TaskotronValueError('path and localpath lists must be of the same '
121 'length.') 135​ 'length.')
122 136​
123 target_path = params['localpath'] 137​ target_path = params['localpath']
124 138​
125 139​ format_fields = {
140​ 'package': params['package'],
141​ 'gitref': gitref,
142​ 'namespace': namespace,
143​ 'baseurl': baseurl,
144​ }
126 output_data['downloaded_files'] = [] 145​ output_data['downloaded_files'] = []
127 for path, localpath in zip(params['path'], target_path): 146​ for path, localpath in zip(params['path'], target_path):
128 localpath = os.path.join(target_dir, localpath) 147​ localpath = os.path.join(target_dir, localpath)
129 file_utils.makedirs(os.path.dirname(localpath)) 148​ file_utils.makedirs(os.path.dirname(localpath))
149​ url = URL_FMT.format(path=path, **format_fields)
130 output_data['downloaded_files'].append( 150​ output_data['downloaded_files'].append(
131 self._download_file(params['package'], path, localpath) 151​ file_utils.download(url, '.', localpath)
132 ) 152​ )
133 153​
134 return output_data 154​ return output_data
View Options

libtaskotron/main.py

Show All 13 Lines
14import libtaskotron 14​import libtaskotron
15from libtaskotron import logger 15​from libtaskotron import logger
16from libtaskotron import config 16​from libtaskotron import config
17from libtaskotron.logger import log 17​from libtaskotron.logger import log
18from libtaskotron.overlord import Overlord 18​from libtaskotron.overlord import Overlord
19 19​
20 20​
21# The list of accepted item types on the command line (--type option) 21​# The list of accepted item types on the command line (--type option)
22_ITEM_TYPES = ["bodhi_id", "koji_build", "koji_tag", "compose", "docker_image"] 22​_ITEM_TYPES = [
23​ "bodhi_id",
24​ "koji_build",
25​ "koji_tag",
26​ "compose",
27​ "docker_image",
28​ "dist_git_commit",
29​]
23 30​
24 31​
25def get_argparser(): 32​def get_argparser():
26 '''Get the cmdline parser for the main runner. 33​ '''Get the cmdline parser for the main runner.
27 34​
28 :rtype: :class:`argparse.ArgumentParser` 35​ :rtype: :class:`argparse.ArgumentParser`
29 ''' 36​ '''
30 # IMPORTANT: When you add a new argument, look into `minion.BaseMinion.arg_data_exclude` and 37​ # IMPORTANT: When you add a new argument, look into `minion.BaseMinion.arg_data_exclude` and
▲ Show 20 Lines • Show All 140 Lines • Show Last 20 Lines
View Options

testing/test_distgit_directive.py

1# -*- coding: utf-8 -*- 1​# -*- coding: utf-8 -*-
2# Copyright 2009-2015, Red Hat, Inc. 2​# Copyright 2009-2015, 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​
6import os.path 6​import os.path
7import pytest 7​import pytest
8from dingus import Dingus 8​from dingus import Dingus
9 9​
10from libtaskotron import file_utils 10​from libtaskotron import file_utils
11from libtaskotron.directives import distgit_directive 11​from libtaskotron.directives import distgit_directive
12import libtaskotron.exceptions as exc 12​import libtaskotron.exceptions as exc
13 13​
14 14​
15class TestDistGitDirective(): 15​class TestDistGitDirective():
16 def setup_method(self, method): 16​ def setup_method(self, method):
17 self.ref_nvr = 'foo-1.2-3.fc99' 17​ self.ref_name = 'foo'
18 self.ref_path = ['foo.spec'] 18​ self.ref_path = ['foo.spec']
19 self.ref_localpath = ['local.foo.spec'] 19​ self.ref_localpath = ['local.foo.spec']
20 self.ref_branch = 'f99' 20​ self.ref_branch = 'f99'
21 self.ref_arg_data = {'workdir': '/var/tmp/foo'} 21​ self.ref_arg_data = {'workdir': '/var/tmp/foo'}
22 self.helper = distgit_directive.DistGitDirective() 22​ self.helper = distgit_directive.DistGitDirective()
23 23​
24 def _get_url(self, path): 24​ def _get_url(self, path):
25 return distgit_directive.URL_FMT % ('foo', path, self.ref_branch) 25​ return distgit_directive.URL_FMT.format(
26​ baseurl='http://pkgs.fedoraproject.org',
27​ namespace='rpms',
28​ package='foo',
29​ path=path,
30​ gitref=self.ref_branch)
26 31​
27 def test_parse_download_command(self, monkeypatch): 32​ def test_parse_download_command(self, monkeypatch):
28 ref_input = {'action': 'download', 33​ ref_input = {'action': 'download',
29 'package': self.ref_nvr, 34​ 'package': self.ref_name,
35​ 'gitref': self.ref_branch,
30 'path': self.ref_path} 36​ 'path': self.ref_path}
31 37​
32 monkeypatch.setattr(file_utils, 'download', Dingus()) 38​ monkeypatch.setattr(file_utils, 'download', Dingus())
33 39​
34 self.helper.process(ref_input, self.ref_arg_data) 40​ self.helper.process(ref_input, self.ref_arg_data)
35 41​
36 download_calls = file_utils.download.calls 42​ download_calls = file_utils.download.calls
37 43​
38 assert len(download_calls) == 1 44​ assert len(download_calls) == 1
39 assert download_calls[0][1][0] == self._get_url(self.ref_path[0]) 45​ assert download_calls[0][1][0] == self._get_url(self.ref_path[0])
40 assert download_calls[0][1][2] == os.path.join(self.ref_arg_data['workdir'], 46​ assert download_calls[0][1][2] == os.path.join(self.ref_arg_data['workdir'],
41 self.ref_path[0]) 47​ self.ref_path[0])
42 48​
43 def test_parse_download_command_localpath(self, monkeypatch): 49​ def test_parse_download_command_localpath(self, monkeypatch):
44 ref_input = {'action': 'download', 50​ ref_input = {'action': 'download',
45 'package': self.ref_nvr, 51​ 'package': self.ref_name,
52​ 'gitref': self.ref_branch,
46 'path': self.ref_path, 53​ 'path': self.ref_path,
47 'localpath': self.ref_localpath} 54​ 'localpath': self.ref_localpath}
48 55​
49 monkeypatch.setattr(file_utils, 'download', Dingus()) 56​ monkeypatch.setattr(file_utils, 'download', Dingus())
50 57​
51 self.helper.process(ref_input, self.ref_arg_data) 58​ self.helper.process(ref_input, self.ref_arg_data)
52 59​
53 download_calls = file_utils.download.calls 60​ download_calls = file_utils.download.calls
54 61​
55 assert len(download_calls) == 1 62​ assert len(download_calls) == 1
56 assert download_calls[0][1][0] == self._get_url(self.ref_path[0]) 63​ assert download_calls[0][1][0] == self._get_url(self.ref_path[0])
57 assert download_calls[0][1][2] == os.path.join(self.ref_arg_data['workdir'], 64​ assert download_calls[0][1][2] == os.path.join(self.ref_arg_data['workdir'],
58 self.ref_localpath[0]) 65​ self.ref_localpath[0])
59 66​
60 def test_parse_download_command_multiple_localpath(self, monkeypatch): 67​ def test_parse_download_command_multiple_localpath(self, monkeypatch):
61 ref_path = ['file1', 'file2'] 68​ ref_path = ['file1', 'file2']
62 ref_localpath = ['file3', 'file4'] 69​ ref_localpath = ['file3', 'file4']
63 ref_input = {'action': 'download', 70​ ref_input = {'action': 'download',
64 'package': self.ref_nvr, 71​ 'package': self.ref_name,
72​ 'gitref': self.ref_branch,
65 'path': ref_path, 73​ 'path': ref_path,
66 'localpath': ref_localpath} 74​ 'localpath': ref_localpath}
67 75​
68 monkeypatch.setattr(file_utils, 'download', Dingus()) 76​ monkeypatch.setattr(file_utils, 'download', Dingus())
69 77​
70 self.helper.process(ref_input, self.ref_arg_data) 78​ self.helper.process(ref_input, self.ref_arg_data)
71 79​
72 download_calls = file_utils.download.calls 80​ download_calls = file_utils.download.calls
73 81​
74 assert len(download_calls) == 2 82​ assert len(download_calls) == 2
75 assert download_calls[0][1][0] == self._get_url(ref_path[0]) 83​ assert download_calls[0][1][0] == self._get_url(ref_path[0])
76 assert download_calls[1][1][0] == self._get_url(ref_path[1]) 84​ assert download_calls[1][1][0] == self._get_url(ref_path[1])
77 assert download_calls[0][1][2] == os.path.join(self.ref_arg_data['workdir'], 85​ assert download_calls[0][1][2] == os.path.join(self.ref_arg_data['workdir'],
78 ref_localpath[0]) 86​ ref_localpath[0])
79 assert download_calls[1][1][2] == os.path.join(self.ref_arg_data['workdir'], 87​ assert download_calls[1][1][2] == os.path.join(self.ref_arg_data['workdir'],
80 ref_localpath[1]) 88​ ref_localpath[1])
81 89​
82 def test_parse_download_command_incorrect_localpath(self): 90​ def test_parse_download_command_incorrect_localpath(self):
83 ref_path = ['file1', 'file2'] 91​ ref_path = ['file1', 'file2']
84 ref_localpath = ['file3'] 92​ ref_localpath = ['file3']
85 ref_input = {'action': 'download', 93​ ref_input = {'action': 'download',
86 'package': self.ref_nvr, 94​ 'package': self.ref_name,
95​ 'gitref': self.ref_branch,
87 'path': ref_path, 96​ 'path': ref_path,
88 'localpath': ref_localpath} 97​ 'localpath': ref_localpath}
89 98​
90 with pytest.raises(exc.TaskotronValueError): 99​ with pytest.raises(exc.TaskotronValueError):
91 self.helper.process(ref_input, self.ref_arg_data) 100​ self.helper.process(ref_input, self.ref_arg_data)
92 101​
93 def test_parse_download_command_incorrect_path_str(self): 102​ def test_parse_download_command_incorrect_path_str(self):
94 ref_path = 'file1' 103​ ref_path = 'file1'
95 ref_input = {'action': 'download', 104​ ref_input = {'action': 'download',
96 'package': self.ref_nvr, 105​ 'package': self.ref_name,
106​ 'gitref': self.ref_branch,
97 'path': ref_path} 107​ 'path': ref_path}
98 108​
99 with pytest.raises(exc.TaskotronValueError): 109​ with pytest.raises(exc.TaskotronValueError):
100 self.helper.process(ref_input, self.ref_arg_data) 110​ self.helper.process(ref_input, self.ref_arg_data)
101 111​
102 def test_parse_download_command_incorrect_localpath_str(self): 112​ def test_parse_download_command_incorrect_localpath_str(self):
103 ref_localpath = 'file2' 113​ ref_localpath = 'file2'
104 ref_input = {'action': 'download', 114​ ref_input = {'action': 'download',
105 'package': self.ref_nvr, 115​ 'package': self.ref_name,
116​ 'gitref': self.ref_branch,
106 'path': self.ref_path, 117​ 'path': self.ref_path,
107 'localpath': ref_localpath} 118​ 'localpath': ref_localpath}
108 119​
109 with pytest.raises(exc.TaskotronValueError): 120​ with pytest.raises(exc.TaskotronValueError):
110 self.helper.process(ref_input, self.ref_arg_data) 121​ self.helper.process(ref_input, self.ref_arg_data)
111 122​
112 def test_parse_download_command_missing_package(self): 123​ def test_parse_download_command_missing_package(self):
113 ref_input = {'action': 'download', 124​ ref_input = {'action': 'download',
114 'path': self.ref_path} 125​ 'path': self.ref_path}
115 126​
116 with pytest.raises(exc.TaskotronDirectiveError): 127​ with pytest.raises(exc.TaskotronDirectiveError):
117 self.helper.process(ref_input, self.ref_arg_data) 128​ self.helper.process(ref_input, self.ref_arg_data)
118 129​
119 def test_parse_download_command_missing_path(self): 130​ def test_parse_download_command_missing_path(self):
120 ref_input = {'action': 'download', 131​ ref_input = {'action': 'download',
121 'package': self.ref_nvr} 132​ 'package': self.ref_name,
133​ 'gitref': self.ref_branch}
122 134​
123 with pytest.raises(exc.TaskotronDirectiveError): 135​ with pytest.raises(exc.TaskotronDirectiveError):
124 self.helper.process(ref_input, self.ref_arg_data) 136​ self.helper.process(ref_input, self.ref_arg_data)