Differential D136

T216 - Changes blocking depcheck
ClosedPublic
Actions

Authored by jskladan on Jun 12 2014, 12:24 PM.

Details

Reviewers
kparal
tflink
mkrizek
Maniphest Tasks
T216: Fix taskotron issues preventing depcheck from running properly
Commits
rLTRN14b08fa55507: T216 - Changes blocking depcheck
Summary

Small but important fixes for bugs found when trying to make depcheck run via taskotron

Test Plan

unit tests are updated

Diff Detail

Repository
rLTRN libtaskotron
Lint
Lint Skipped
Unit
Unit Tests Skipped
jskladan retitled this revision from to T216 - Changes blocking depcheck.Jun 12 2014, 12:24 PM
jskladan updated this object.
jskladan edited the test plan for this revision. (Show Details)
jskladan added reviewers: tflink, kparal, mkrizek.
tflink requested changes to this revision.Jun 12 2014, 1:44 PM

Changes look good overall but I'd like to see more detail in the exceptions raised.

libtaskotron/directives/yumrepoinfo_directive.py
64

I see why you've written it this way but I'm wondering if separating the arch validity checks would make for better error messages. Something like

processed_arches = [arch for arch in arches if arch not in ['noarch', 'all', 'src']]
if len(processed_arches) == 0:
  raise TaskotronDirectiveError("No valid yumrepo arches supplied to yumrepoinfo directive. Recieved %s" % str(arches))

if len(processed_arches) > 1:
  raise TaskotronDirectiveError("Yumrepoinfo requires a single arch but multiple arches were submitted: %s" % arches)
libtaskotron/koji_utils.py
100

can you add more detail to this exception? Something about which directory should have been created?

This revision now requires changes to proceed.Jun 12 2014, 1:44 PM
kparal added inline comments.Jun 12 2014, 1:50 PM
libtaskotron/directives/yumrepoinfo_directive.py
64–65

Maybe you could be more descriptive here, like "Expected just a single major architecture" or something like that.

libtaskotron/koji_utils.py
84–97

If this is really blocking you, I'm ACK for this for the moment, and please create a ticket so that it's fixed properly in the future. I think file_utils.download() should get changed to prevent these problems.

jskladan updated this revision to Diff 399.Jun 12 2014, 1:59 PM

Updated according to reviews

kparal accepted this revision.Jun 12 2014, 2:00 PM
tflink accepted this revision.Jun 12 2014, 2:15 PM

Looks good to me

This revision is now accepted and ready to land.Jun 12 2014, 2:15 PM
jskladan closed this revision.Jun 12 2014, 2:46 PM
jskladan updated this revision to Diff 402.

Closed by commit rLTRN14b08fa55507 (authored by @jskladan).

Revision Contents

Diff 402

View Options

libtaskotron/directives/yumrepoinfo_directive.py

Show First 20 Lines • Show All 49 Lines • ▼ Show 20 Line(s)49 def __init__(self, repoinfo = None):
50 self.repoinfo = repoinfo 50​ self.repoinfo = repoinfo
51 51​
52 def process(self, input_data, env_data): 52​ def process(self, input_data, env_data):
53 53​
54 if 'koji_tag' not in input_data or 'arch' not in input_data: 54​ if 'koji_tag' not in input_data or 'arch' not in input_data:
55 raise TaskotronDirectiveError("The yumrepoinfo directive requires "\ 55​ raise TaskotronDirectiveError("The yumrepoinfo directive requires "\
56 "koji_tag and arch arguments.") 56​ "koji_tag and arch arguments.")
57 57​
58 arch = input_data['arch'] 58​
59​ arches = input_data['arch']
60​
61​ processed_arches = [arch for arch in arches if arch not in ['noarch', 'all', 'src']]
62​ if len(processed_arches) == 0:
63​ raise TaskotronDirectiveError("No valid yumrepo arches supplied to "
64​ "yumrepoinfo directive. Recieved %r" % arches)
tflinkUnsubmitted
Not Done

I see why you've written it this way but I'm wondering if separating the arch validity checks would make for better error messages. Something like

processed_arches = [arch for arch in arches if arch not in ['noarch', 'all', 'src']]
if len(processed_arches) == 0:
  raise TaskotronDirectiveError("No valid yumrepo arches supplied to yumrepoinfo directive. Recieved %s" % str(arches))

if len(processed_arches) > 1:
  raise TaskotronDirectiveError("Yumrepoinfo requires a single arch but multiple arches were submitted: %s" % arches)
65​
kparalUnsubmitted
Not Done

Maybe you could be more descriptive here, like "Expected just a single major architecture" or something like that.

66​ if len(processed_arches) > 1:
67​ raise TaskotronDirectiveError("Yumrepoinfo requires a single arch "
68​ "but multiple arches were submitted: %r" % arches)
69​
70​ arch = processed_arches[0]
71​
59 koji_tag = input_data['koji_tag'] 72​ koji_tag = input_data['koji_tag']
60 73​
61 output = {} 74​ output = {}
62 75​
63 if self.repoinfo is None: 76​ if self.repoinfo is None:
64 self.repoinfo = yumrepoinfo.get_yumrepoinfo(arch) 77​ self.repoinfo = yumrepoinfo.get_yumrepoinfo(arch)
65 78​
66 if koji_tag.endswith('-pending'): 79​ if koji_tag.endswith('-pending'):
Show All 17 Lines
View Options

libtaskotron/koji_utils.py

1# -*- coding: utf-8 -*- 1​# -*- coding: utf-8 -*-
2# Copyright 2014, Red Hat, Inc. 2​# Copyright 2014, Red Hat, Inc.
3# License: GNU General Public License version 2 or later 3​# License: GNU General Public License version 2 or later
4 4​
5''' Utility methods related to Koji ''' 5​''' Utility methods related to Koji '''
6 6​
7from __future__ import absolute_import 7​from __future__ import absolute_import
8​import os
9​
8import collections 10​import collections
9import koji 11​import koji
10import hawkey 12​import hawkey
11 13​
12from libtaskotron import file_utils 14​from libtaskotron import file_utils
13from libtaskotron.logger import log 15​from libtaskotron.logger import log
14from libtaskotron import exceptions as exc 16​from libtaskotron import exceptions as exc
15from libtaskotron import rpm_utils 17​from libtaskotron import rpm_utils
▲ Show 20 Lines • Show All 57 Lines • ▼ Show 20 Line(s)74 '''Retrieve the RPMs associated with a build NVR into the specified
73 directory. 75​ directory.
74 76​
75 :param str nvr build: NVR 77​ :param str nvr build: NVR
76 :param str rpm_dir: location where to store the RPMs 78​ :param str rpm_dir: location where to store the RPMs
77 :return: list of local filenames of the grabbed RPMs 79​ :return: list of local filenames of the grabbed RPMs
78 :rtype: list of str 80​ :rtype: list of str
79 :raise TaskotronRemoteError: if downloading failed 81​ :raise TaskotronRemoteError: if downloading failed
80 ''' 82​ '''
83​
84​ # Create the rpm_dir if if does not exist
85​ # as fileutils.download requires existing directory
86​ # If this is not present, then the first rpm to be downloaded
87​ # will fail to download, because of how file_utils.download()
88​ # is currently written. On line #79, the `dest` is not a dir
89​ # (because it is not yet created), so it is created on line #83
90​ # but as an effect, `dest` is still containing just the dirname
91​ # instead of dirname+rpmname (because line #80 was not executed
92​ # so the lines #89-94 end up going through the else branch
93​ # thus not downloading the first file.
94​ # All subsequent downloads are OK, since the rpm_dir (aka dest)
95​ # already exists.
96​ # YAY for two hours of debugging!
97​ # FIXME: this should be solved better, somehow...
kparalUnsubmitted
Not Done

If this is really blocking you, I'm ACK for this for the moment, and please create a ticket so that it's fixed properly in the future. I think file_utils.download() should get changed to prevent these problems.

98​ if os.path.exists(rpm_dir):
99​ if not os.path.isdir(rpm_dir):
100​ raise exc.TaskotronRemoteError(
tflinkUnsubmitted
Not Done

can you add more detail to this exception? Something about which directory should have been created?

101​ "Can't create directory: %r It is an already "
102​ "existing file.", rpm_dir)
103​ else:
104​ try:
105​ file_utils.makedirs(rpm_dir)
106​ except OSError, e:
107​ log.exception("Can't create directory: %s", rpm_dir)
108​ raise TaskotronRemoteError(e)
109​
110​
81 rpm_urls = self.nvr_to_urls(nvr, 111​ rpm_urls = self.nvr_to_urls(nvr,
82 arches=arches, 112​ arches=arches,
83 debuginfo=debuginfo, 113​ debuginfo=debuginfo,
84 src=src) 114​ src=src)
85 rpm_files = [] 115​ rpm_files = []
86 log.info('Fetching RPMs for: %s', nvr) 116​ log.info('Fetching RPMs for: %s', nvr)
87 for url in rpm_urls: 117​ for url in rpm_urls:
88 log.info(' fetching %s', url) 118​ log.info(' fetching %s', url)
▲ Show 20 Lines • Show All 46 Lines • Show Last 20 Lines
View Options

testing/test_yumrepoinfo_directive.py

Show First 20 Lines • Show All 42 Lines • ▼ Show 20 Line(s)
43 43​
44 44​
45class TestYumrepoinfoDirective(object): 45​class TestYumrepoinfoDirective(object):
46 46​
47 @classmethod 47​ @classmethod
48 def setup_class(cls): 48​ def setup_class(cls):
49 '''One-time class initialization''' 49​ '''One-time class initialization'''
50 # create YumRepoInfo initialized with TEST_CONF 50​ # create YumRepoInfo initialized with TEST_CONF
51 cls.ref_arch = 'x86_64' 51​ cls.ref_arch = ['x86_64']
52 cls.repoinfo = yumrepoinfo.YumRepoInfo(filelist=[], arch=cls.ref_arch) 52​ cls.repoinfo = yumrepoinfo.YumRepoInfo(filelist=[], arch=cls.ref_arch)
53 cls.repoinfo.parser.readfp(StringIO.StringIO(TEST_CONF)) 53​ cls.repoinfo.parser.readfp(StringIO.StringIO(TEST_CONF))
54 54​
55 def test_missing_kojitag(self): 55​ def test_missing_kojitag(self):
56 directive = yumrepoinfo_directive.YumrepoinfoDirective(self.repoinfo) 56​ directive = yumrepoinfo_directive.YumrepoinfoDirective(self.repoinfo)
57 ref_input = {"arch":"x86_64"} 57​ ref_input = {"arch": ["x86_64"]}
58 58​
59 with pytest.raises(TaskotronDirectiveError): 59​ with pytest.raises(TaskotronDirectiveError):
60 directive.process(ref_input, None) 60​ directive.process(ref_input, None)
61 61​
62 def test_missing_arch(self): 62​ def test_missing_arch(self):
63 directive = yumrepoinfo_directive.YumrepoinfoDirective(self.repoinfo) 63​ directive = yumrepoinfo_directive.YumrepoinfoDirective(self.repoinfo)
64 ref_input = {"koji_tag":"rawhide"} 64​ ref_input = {"koji_tag":"rawhide"}
65 65​
66 with pytest.raises(TaskotronDirectiveError): 66​ with pytest.raises(TaskotronDirectiveError):
67 directive.process(ref_input, None) 67​ directive.process(ref_input, None)
68 68​
69 def test_pending(self): 69​ def test_pending(self):
70 directive = yumrepoinfo_directive.YumrepoinfoDirective(self.repoinfo) 70​ directive = yumrepoinfo_directive.YumrepoinfoDirective(self.repoinfo)
71 ref_input = {"koji_tag": "f20-pending", "arch": "x86_64"} 71​ ref_input = {"koji_tag": "f20-pending", "arch": ["x86_64"]}
72 72​
73 output = directive.process(ref_input, None) 73​ output = directive.process(ref_input, None)
74 74​
75 assert output == {"f20": "http://download.fedoraproject.org/pub/fedora/linux/releases/20/Everything/x86_64/os"} 75​ assert output == {"f20": "http://download.fedoraproject.org/pub/fedora/linux/releases/20/Everything/x86_64/os"}
76 76​
77 def test_rawhide(self): 77​ def test_rawhide(self):
78 directive = yumrepoinfo_directive.YumrepoinfoDirective(self.repoinfo) 78​ directive = yumrepoinfo_directive.YumrepoinfoDirective(self.repoinfo)
79 ref_input = {"koji_tag": "rawhide", "arch": "x86_64"} 79​ ref_input = {"koji_tag": "rawhide", "arch": ["x86_64"]}
80 80​
81 output = directive.process(ref_input, None) 81​ output = directive.process(ref_input, None)
82 82​
83 assert output == {"rawhide": "http://download.fedoraproject.org/pub/fedora/linux/development/rawhide/x86_64/os"} 83​ assert output == {"rawhide": "http://download.fedoraproject.org/pub/fedora/linux/development/rawhide/x86_64/os"}
84 84​
85 def test_bad_kojitag(self): 85​ def test_bad_kojitag(self):
86 directive = yumrepoinfo_directive.YumrepoinfoDirective(self.repoinfo) 86​ directive = yumrepoinfo_directive.YumrepoinfoDirective(self.repoinfo)
87 ref_input = {"koji_tag": "my random tag"} 87​ ref_input = {"koji_tag": "my random tag"}
88 88​
89 with pytest.raises(TaskotronDirectiveError): 89​ with pytest.raises(TaskotronDirectiveError):
90 directive.process(ref_input, None) 90​ directive.process(ref_input, None)
91 91​
92 def test_repo_path(self): 92​ def test_repo_path(self):
93 93​
94 directive = yumrepoinfo_directive.YumrepoinfoDirective(self.repoinfo) 94​ directive = yumrepoinfo_directive.YumrepoinfoDirective(self.repoinfo)
95 ref_input = {"koji_tag": "f20-updates", "arch": "x86_64"} 95​ ref_input = {"koji_tag": "f20-updates", "arch": ["x86_64"]}
96 96​
97 output = directive.process(ref_input, None) 97​ output = directive.process(ref_input, None)
98 98​
99 assert output == { 99​ assert output == {
100 "f20": "http://download.fedoraproject.org/pub/fedora/linux/releases/20/Everything/x86_64/os", 100​ "f20": "http://download.fedoraproject.org/pub/fedora/linux/releases/20/Everything/x86_64/os",
101 "f20-updates": "http://download.fedoraproject.org/pub/fedora/linux/updates/20/x86_64", 101​ "f20-updates": "http://download.fedoraproject.org/pub/fedora/linux/updates/20/x86_64",
102 } 102​ }
103 103​
104 def test_use_arch(self, monkeypatch): 104​ def test_use_arch(self, monkeypatch):
105 """Make sure that the arch passed in as an arg is used to create the 105​ """Make sure that the arch passed in as an arg is used to create the
106 yumrepoinfo object instead of falling back to the default system arch""" 106​ yumrepoinfo object instead of falling back to the default system arch"""
107 ref_arch = 'i386' 107​ ref_arch = 'i386'
108 108​
109 stub_getrepoinfo = Dingus(return_value=self.repoinfo) 109​ stub_getrepoinfo = Dingus(return_value=self.repoinfo)
110 monkeypatch.setattr(yumrepoinfo, 'get_yumrepoinfo', stub_getrepoinfo) 110​ monkeypatch.setattr(yumrepoinfo, 'get_yumrepoinfo', stub_getrepoinfo)
111 111​
112 # don't set the repoinfo object, we've stubbed out the code that would 112​ # don't set the repoinfo object, we've stubbed out the code that would
113 # hit the filesystem, so it's not a risk here 113​ # hit the filesystem, so it's not a risk here
114 directive = yumrepoinfo_directive.YumrepoinfoDirective() 114​ directive = yumrepoinfo_directive.YumrepoinfoDirective()
115 ref_input = {"koji_tag": "f20-updates", "arch": ref_arch} 115​ ref_input = {"koji_tag": "f20-updates", "arch": [ref_arch]}
116 116​
117 directive.process(ref_input, None) 117​ directive.process(ref_input, None)
118 118​
119 # check the first arg of the first call to the stub object 119​ # check the first arg of the first call to the stub object
120 assert stub_getrepoinfo.calls()[0][1][0] == ref_arch 120​ assert stub_getrepoinfo.calls()[0][1][0] == ref_arch
121 121​
122 122​