Differential D942

modify the libtaskotron code to support the change in task-rpmlint
AbandonedPublic
Actions

Authored by lnie on Jul 20 2016, 3:35 AM.

Details

Reviewers
lbrabec
kparal
jskladan
Summary

modify the libtaskotron code to support the change in task-rpmlint

Test Plan

just run the normal rpmlint task with bodhi_id

Diff Detail

Repository
rLTRN libtaskotron
Branch
angel
Lint
Lint OK
Unit
Unit Test Errors
Build Status
Buildable 736
Build 736: arc lint + arc unit

Unit TestsFailed
View All

Excuse: prospective failures,as I have changed 'koji_build' to 'koji_bodhi' to validate bodhi_id item_type.
TimeTest
1 mstesting.functest_logger.TestLogger.test_logfile_no_write
0 mstesting.test_koji_directive.TestKojiDirective.test_add_noarch[download]
0 mstesting.test_koji_directive.TestKojiDirective.test_add_noarch[download_latest_stable]
0 mstesting.test_koji_directive.TestKojiDirective.test_build_log_only_for_certain_actions[download]
0 mstesting.test_koji_directive.TestKojiDirective.test_build_log_only_for_certain_actions[download_latest_stable]
View Full Test Results (19 Failed · 453 Passed)
lnie retitled this revision from to modify the libtaskotron code to support the change in task-rpmlint.Jul 20 2016, 3:35 AM
lnie updated this object.
lnie edited the test plan for this revision. (Show Details)
lnie added reviewers: jskladan, kparal, lbrabec.
jskladan added a comment.EditedJul 20 2016, 6:04 AM

Some comments on the code, as I think the implementation deserves some comments since you are learning, and plain "no" would not be helpful. That said, I am absolutely unconvinced that this whole idea is sound, or has merit. I don't see any actual advantage, and have not been shown one so far, nor does this fix any reported issue.

libtaskotron/directives/koji_directive.py
59–222

It does not make sense to change the parameter name in the koji directive, it is not semantic, and suggests other behaviour than what is implemented (seems like you could download koji build based on bodhi update id, but that is not true. It also does not need to be changed in respect to the rpmlint patch, you could just put a different variable (koji_bodhi in your case) as a value to the koji_build (or update_id in the bodhi directive case) without changing the actual parameter name.

libtaskotron/executor.py
137–143

Absolutely not - you can not just silently "do nothing" without even a warning log message. If you wanted to do this, then this is not event the right place - it should be in the respective directive code, and not "automatic" - there should be an option in that directives (something like dont_fail, set to Falseby default), that can be overriden in that one specific case, where you don't want the directive to just plain fail. It would also not be driven by the item_type at all, it would just make sure that when an update/build is not found, the directive won't fail. Which in itself has problems, and I'd argue against the whole concept of it.

libtaskotron/main.py
112

If you wanted to do what this seems to want to do, then you should explicitly check whether args['type'] is in [KOJI_BUILD, BODHI_UPDATE], and populate the koji_bodhi only then.

jskladan requested changes to this revision.EditedJul 20 2016, 6:06 AM

double post

This revision now requires changes to proceed.Jul 20 2016, 6:06 AM
jskladan added a comment.Jul 20 2016, 6:09 AM

Also - before you post a patch for review, you should make sure that all unittests are passing. This amount of failed tests is a reason to nack the revision on its own, without further review.

lnie added inline comments.Jul 20 2016, 8:33 AM
libtaskotron/directives/koji_directive.py
59–222

I see what you are talking about,but I was just wanting to take full advantage of task-rpmlint,and add a bodhi_id support for rpmlint.

libtaskotron/executor.py
137–143

Maybe we can just close this diff ,if it will takes so much time and need so complex change.

libtaskotron/main.py
112

Good suggestion: )

kparal added a comment.Jul 20 2016, 1:46 PM

I assume this is tightly related to D939 (next time, you can use Edit dependencies to link the tickets together -- I just did it now; or at least mention it in the description). If that's the case, I think we can do the same as for D939 - choose Abandon revision. If I understood it correctly, this is not something that we want in libtaskotron right now.

lnie added a comment.Jul 22 2016, 7:05 AM
In D942#17762, @kparal wrote:

I assume this is tightly related to D939 (next time, you can use Edit dependencies to link the tickets together -- I just did it now; or at least mention it in the description).

yes,and thanks for doing that.Actually,I added it ,but I removed it later,cause it reminded me of the dead lock:D939 can't be closed until D942 is closed,vice versa(now I know I have thought too much^^),and considering people can see my 'add and remove' action history,I thought it's fine to leave it alone.

lnie abandoned this revision.Jul 22 2016, 7:05 AM

Revision Contents

PathCoverage (All)Coverage (Touched)Packages
Mlibtaskotron/directives/koji_directive.py (22 lines)73%40%
Mlibtaskotron/executor.py (7 lines)90%60%
Mlibtaskotron/main.py (2 lines)94%100%

Diff 2391

View Options

libtaskotron/directives/koji_directive.py

Show First 20 Lines • Show All 50 Lines • ▼ Show 20 Line(s)47 description: download also ``build.log`` files for each requested architecture (the files will
51 ``action='download_latest_stable'``). 51​ ``action='download_latest_stable'``).
52 type: bool 52​ type: bool
53 default: False 53​ default: False
54 debuginfo: 54​ debuginfo:
55 required: false 55​ required: false
56 description: download also ``debuginfo`` RPM files 56​ description: download also ``debuginfo`` RPM files
57 type: bool 57​ type: bool
58 default: False 58​ default: False
59 koji_build: 59​ koji_bodhi:
60 required: true 60​ required: true
61 description: | 61​ description: |
62 N(E)VR of a Koji build to download (for ``action="download"``) or to search 62​ N(E)VR of a Koji build to download (for ``action="download"``) or to search
63 the latest stable build for (for ``action="download_latest_stable"``). Not 63​ the latest stable build for (for ``action="download_latest_stable"``). Not
64 required for ``action="download_tag"``. Example: ``xchat-2.8.8-21.fc20`` 64​ required for ``action="download_tag"``. Example: ``xchat-2.8.8-21.fc20``
65 type: str 65​ type: str
66 koji_tag: 66​ koji_tag:
67 required: true 67​ required: true
68 description: | 68​ description: |
69 name of a Koji tag to download all builds from. Only required when 69​ name of a Koji tag to download all builds from. Only required when
70 ``action="download_tag"``. Example: ``f20-updates-pending`` 70​ ``action="download_tag"``. Example: ``f20-updates-pending``
71 type: str 71​ type: str
72 src: 72​ src:
73 required: false 73​ required: false
74 description: download also ``src`` RPM files 74​ description: download also ``src`` RPM files
75 type: bool 75​ type: bool
76 default: False 76​ default: False
77 target_dir: 77​ target_dir:
78 required: false 78​ required: false
79 description: directory into which to download builds 79​ description: directory into which to download builds
80 type: str 80​ type: str
81 default: ${workdir} 81​ default: ${workdir}
82returns: | 82​returns: |
83 A dictionary containing following items: 83​ A dictionary containing following items:
84 84​
85 * `downloaded_rpms`: (list of str) a list of absolute paths of the downloaded RPMs 85​ * `downloaded_rpms`: (list of str) a list of absolute paths of the downloaded RPMs
86 * `downloaded_logs`: (list of str) a list of absolute paths of the downloaded build logs 86​ * `downloaded_logs`: (list of str) a list of absolute paths of the downloaded build logs
87 * `log_errors`: (list of str) a list of architectures for which the ``build.log`` could not be 87​ * `log_errors`: (list of str) a list of architectures for which the ``build.log`` could not be
88 downloaded 88​ downloaded
89raises: | 89​raises: |
90 * :class:`.TaskotronRemoteError`: if downloading failed 90​ * :class:`.TaskotronRemoteError`: if downloading failed
91version_added: 0.4 91​version_added: 0.4
92""" 92​"""
93 93​
94EXAMPLES = """ 94​EXAMPLES = """
95Rpmlint needs to download a specific build from Koji, all architectures 95​Rpmlint needs to download a specific build from Koji, all architectures
96including src.rpm:: 96​including src.rpm::
97 97​
98 - name: download rpms from koji 98​ - name: download rpms from koji
99 koji: 99​ koji:
100 action: download 100​ action: download
101 koji_build: ${koji_build} 101​ koji_bodhi: ${koji_bodhi}
102 arch: ['all'] 102​ arch: ['all']
103 src: True 103​ src: True
104 104​
105Depcheck needs to download all builds in a specific Koji tag for the current 105​Depcheck needs to download all builds in a specific Koji tag for the current
106architecture:: 106​architecture::
107 107​
108 - name: download koji tag 108​ - name: download koji tag
109 koji: 109​ koji:
110 action: download_tag 110​ action: download_tag
111 koji_tag: ${koji_tag} 111​ koji_tag: ${koji_tag}
112 arch: [${arch}] 112​ arch: [${arch}]
113 target_dir: ${workdir}/downloaded_tag/ 113​ target_dir: ${workdir}/downloaded_tag/
114 114​
115Abicheck downloads both current build and latest stable build. It checks all binary arches, but 115​Abicheck downloads both current build and latest stable build. It checks all binary arches, but
116doesn't need ``noarch``:: 116​doesn't need ``noarch``::
117 117​
118 - name: download latest stable rpms for the package 118​ - name: download latest stable rpms for the package
119 koji: 119​ koji:
120 action: download_latest_stable 120​ action: download_latest_stable
121 koji_build: ${koji_build} 121​ koji_bodhi: ${koji_bodhi}
122 arch: ['all'] 122​ arch: ['all']
123 arch_exclude: ['noarch'] 123​ arch_exclude: ['noarch']
124 debuginfo: True 124​ debuginfo: True
125 target_dir: ${workdir}/stable 125​ target_dir: ${workdir}/stable
126 126​
127 - name: download rpms for the tested build 127​ - name: download rpms for the tested build
128 koji: 128​ koji:
129 action: download 129​ action: download
130 koji_build: ${koji_build} 130​ koji_bodhi: ${koji_bodhi}
131 arch: ['all'] 131​ arch: ['all']
132 arch_exclude: ['noarch'] 132​ arch_exclude: ['noarch']
133 debuginfo: True 133​ debuginfo: True
134 target_dir: ${workdir}/update 134​ target_dir: ${workdir}/update
135 135​
136""" 136​"""
137 137​
138from libtaskotron.directives import BaseDirective 138​from libtaskotron.directives import BaseDirective
139import libtaskotron.exceptions as exc 139​import libtaskotron.exceptions as exc
140from libtaskotron.ext.fedora.koji_utils import KojiClient 140​from libtaskotron.ext.fedora.koji_utils import KojiClient
141from libtaskotron.ext.fedora import rpm_utils 141​from libtaskotron.ext.fedora import rpm_utils
142from libtaskotron.logger import log 142​from libtaskotron.logger import log
143 143​
144 144​
145directive_class = 'KojiDirective' 145​directive_class = 'KojiDirective'
146 146​
147 147​
148class KojiDirective(BaseDirective): 148​class KojiDirective(BaseDirective):
149 149​
150 def __init__(self, koji_session=None): 150​ def __init__(self, koji_session=None):
151 super(KojiDirective, self).__init__() 151​ super(KojiDirective, self).__init__()
152 if koji_session is None: 152​ if koji_session is None:
153 self.koji = KojiClient() 153​ self.koji = KojiClient()
154 else: 154​ else:
155 self.koji = koji_session 155​ self.koji = koji_session
156 156​
157 def process(self, params, arg_data): 157​ def process(self, params, arg_data):
158 # process params 158​ # process params
159 valid_actions = ['download', 'download_tag', 'download_latest_stable'] 159​ valid_actions = ['download', 'download_tag', 'download_latest_stable']
160 action = params['action'] 160​ action = params['action']
161 if action not in valid_actions: 161​ if action not in valid_actions:
162 raise exc.TaskotronDirectiveError('%s is not a valid action for koji ' 162​ raise exc.TaskotronDirectiveError('%s is not a valid action for koji '
163 'directive' % action) 163​ 'directive' % action)
164 164​
165 if 'target_dir' not in params: 165​ if 'target_dir' not in params:
166 target_dir = arg_data['workdir'] 166​ target_dir = arg_data['workdir']
167 else: 167​ else:
168 target_dir = params['target_dir'] 168​ target_dir = params['target_dir']
169 169​
170 if 'arch' not in params: 170​ if 'arch' not in params:
171 detected_args = ', '.join(params.keys()) 171​ detected_args = ', '.join(params.keys())
172 raise exc.TaskotronDirectiveError( 172​ raise exc.TaskotronDirectiveError(
173 "The koji directive requires 'arch' as an argument. Detected " 173​ "The koji directive requires 'arch' as an argument. Detected "
174 "arguments: %s" % detected_args) 174​ "arguments: %s" % detected_args)
175 175​
176 arches = list(params['arch']) 176​ arches = list(params['arch'])
177 if arches and ('all' not in arches) and ('noarch' not in arches): 177​ if arches and ('all' not in arches) and ('noarch' not in arches):
178 arches.append('noarch') 178​ arches.append('noarch')
179 179​
180 arch_exclude = params.get('arch_exclude', []) 180​ arch_exclude = params.get('arch_exclude', [])
181 debuginfo = params.get('debuginfo', False) 181​ debuginfo = params.get('debuginfo', False)
182 src = params.get('src', False) 182​ src = params.get('src', False)
183 build_log = params.get('build_log', False) 183​ build_log = params.get('build_log', False)
184 184​
185 # download files 185​ # download files
186 output_data = {} 186​ output_data = {}
187 187​
188 if action == 'download': 188​ if action == 'download':
189 if 'koji_build' not in params: 189​ if 'koji_bodhi' not in params:
190 detected_args = ', '.join(params.keys()) 190​ detected_args = ', '.join(params.keys())
191 raise exc.TaskotronDirectiveError( 191​ raise exc.TaskotronDirectiveError(
192 "The koji directive requires 'koji_build' for the 'download' " 192​ "The koji directive requires 'koji_bodhi' for the 'download' "
193 "action. Detected arguments: %s" % detected_args) 193​ "action. Detected arguments: %s" % detected_args)
194 194​
195 nvr = rpm_utils.rpmformat(params['koji_build'], 'nvr') 195​ nvr = rpm_utils.rpmformat(params['koji_bodhi'], 'nvr')
196 196​
197 output_data['downloaded_rpms'] = self.koji.get_nvr_rpms( 197​ output_data['downloaded_rpms'] = self.koji.get_nvr_rpms(
198 nvr, target_dir, arches=arches, arch_exclude=arch_exclude, 198​ nvr, target_dir, arches=arches, arch_exclude=arch_exclude,
199 debuginfo=debuginfo, src=src) 199​ debuginfo=debuginfo, src=src)
200 200​
201 elif action == 'download_tag': 201​ elif action == 'download_tag':
202 if 'koji_tag' not in params: 202​ if 'koji_tag' not in params:
203 detected_args = ', '.join(params.keys()) 203​ detected_args = ', '.join(params.keys())
204 raise exc.TaskotronDirectiveError( 204​ raise exc.TaskotronDirectiveError(
205 "The koji directive requires 'koji_tag' for the 'download_tag' " 205​ "The koji directive requires 'koji_tag' for the 'download_tag' "
206 "action. Detected arguments: %s" % detected_args) 206​ "action. Detected arguments: %s" % detected_args)
207 207​
208 koji_tag = params['koji_tag'] 208​ koji_tag = params['koji_tag']
209 209​
210 output_data['downloaded_rpms'] = self.koji.get_tagged_rpms( 210​ output_data['downloaded_rpms'] = self.koji.get_tagged_rpms(
211 koji_tag, target_dir, arches=arches, arch_exclude=arch_exclude, 211​ koji_tag, target_dir, arches=arches, arch_exclude=arch_exclude,
212 debuginfo=debuginfo, src=src) 212​ debuginfo=debuginfo, src=src)
213 213​
214 elif action == 'download_latest_stable': 214​ elif action == 'download_latest_stable':
215 if 'koji_build' not in params: 215​ if 'koji_bodhi' not in params:
216 detected_args = ', '.join(params.keys()) 216​ detected_args = ', '.join(params.keys())
217 raise exc.TaskotronDirectiveError( 217​ raise exc.TaskotronDirectiveError(
218 "The koji directive requires 'koji_build' for the 'download_latest_stable' " 218​ "The koji directive requires 'koji_bodhi' for the 'download_latest_stable' "
219 "action. Detected arguments: %s" % detected_args) 219​ "action. Detected arguments: %s" % detected_args)
220 220​
221 name = rpm_utils.rpmformat(params['koji_build'], 'n') 221​ name = rpm_utils.rpmformat(params['koji_bodhi'], 'n')
222 disttag = rpm_utils.get_dist_tag(params['koji_build']) 222​ disttag = rpm_utils.get_dist_tag(params['koji_bodhi'])
jskladanUnsubmitted
Not Done

It does not make sense to change the parameter name in the koji directive, it is not semantic, and suggests other behaviour than what is implemented (seems like you could download koji build based on bodhi update id, but that is not true. It also does not need to be changed in respect to the rpmlint patch, you could just put a different variable (koji_bodhi in your case) as a value to the koji_build (or update_id in the bodhi directive case) without changing the actual parameter name.

lnieAuthorUnsubmitted
Not Done

I see what you are talking about,but I was just wanting to take full advantage of task-rpmlint,and add a bodhi_id support for rpmlint.

223 # we need to do 'fc22' -> 'f22' conversion 223​ # we need to do 'fc22' -> 'f22' conversion
224 tag = disttag.replace('c', '') 224​ tag = disttag.replace('c', '')
225 225​
226 # first we need to check updates tag and if that fails, the latest 226​ # first we need to check updates tag and if that fails, the latest
227 # stable nvr is in the base repo 227​ # stable nvr is in the base repo
228 tags = ['%s-updates' % tag, tag] 228​ tags = ['%s-updates' % tag, tag]
229 nvr = self.koji.latest_by_tag(tags, name) 229​ nvr = self.koji.latest_by_tag(tags, name)
230 230​
Show All 16 Lines
View Options

libtaskotron/executor.py

Show First 20 Lines • Show All 128 Lines • ▼ Show 20 Line(s)128 for action in self.formula['actions']:
129 self._do_single_action(action) 129​ self._do_single_action(action)
130 130​
131 def _do_single_action(self, action): 131​ def _do_single_action(self, action):
132 '''Execute a single action from the task. See :meth:`do_actions` to see 132​ '''Execute a single action from the task. See :meth:`do_actions` to see
133 how an action looks like. 133​ how an action looks like.
134 134​
135 :param dict action: An action specification parsed from the task formula 135​ :param dict action: An action specification parsed from the task formula
136 ''' 136​ '''
137​ args = self.arg_data
137 directive_name = self._extract_directive_from_action(action) 138​ directive_name = self._extract_directive_from_action(action)
138 139​ # check whether we should do this single action
140​ if directive_name == 'koji' and args['type'] != 'koji_build':
141​ return
142​ if directive_name == 'bodhi' and args['type'] != 'bodhi_id':
143​ return
jskladanUnsubmitted
Not Done

Absolutely not - you can not just silently "do nothing" without even a warning log message. If you wanted to do this, then this is not event the right place - it should be in the respective directive code, and not "automatic" - there should be an option in that directives (something like dont_fail, set to Falseby default), that can be overriden in that one specific case, where you don't want the directive to just plain fail. It would also not be driven by the item_type at all, it would just make sure that when an update/build is not found, the directive won't fail. Which in itself has problems, and I'd argue against the whole concept of it.

lnieAuthorUnsubmitted
Not Done

Maybe we can just close this diff ,if it will takes so much time and need so complex change.

139 rendered_action = self._render_action(action) 144​ rendered_action = self._render_action(action)
140 145​
141 log.debug("Executing directive: %s", directive_name) 146​ log.debug("Executing directive: %s", directive_name)
142 self._load_directive(directive_name) 147​ self._load_directive(directive_name)
143 148​
144 directive_object = self.directives[directive_name] 149​ directive_object = self.directives[directive_name]
145 directive_classname = directive_object.directive_class 150​ directive_classname = directive_object.directive_class
146 151​
▲ Show 20 Lines • Show All 64 Lines • Show Last 20 Lines
View Options

libtaskotron/main.py

Show First 20 Lines • Show All 103 Lines • ▼ Show 20 Line(s)95def process_args(raw_args):
104 args = copy.deepcopy(raw_args) 104​ args = copy.deepcopy(raw_args)
105 105​
106 # store the original unprocessed args for later use when passing it to the minion 106​ # store the original unprocessed args for later use when passing it to the minion
107 args['_orig_args'] = raw_args 107​ args['_orig_args'] = raw_args
108 108​
109 # process item + type 109​ # process item + type
110 if args['type'] in _ITEM_TYPES: 110​ if args['type'] in _ITEM_TYPES:
111 args[args['type']] = args['item'] 111​ args[args['type']] = args['item']
112 112​ args['koji_bodhi'] = args['item']
jskladanUnsubmitted
Not Done

If you wanted to do what this seems to want to do, then you should explicitly check whether args['type'] is in [KOJI_BUILD, BODHI_UPDATE], and populate the koji_bodhi only then.

lnieAuthorUnsubmitted
Not Done

Good suggestion: )

113 # process arch 113​ # process arch
114 if args['arch'] is None: 114​ if args['arch'] is None:
115 args['arch'] = ['noarch'] 115​ args['arch'] = ['noarch']
116 116​
117 override = {} 117​ override = {}
118 for var in args['override']: 118​ for var in args['override']:
119 name, value = var.split('=', 1) 119​ name, value = var.split('=', 1)
120 override[name] = value 120​ override[name] = value
▲ Show 20 Lines • Show All 50 Lines • Show Last 20 Lines