Software Properties

Merge lp:~hodgestar/software-properties/configurable-key-server into lp:software-properties

  1. configurable-key-server
  2. Merge into main
Proposed by Hodgestar
Status: Merged
Merged at revision: 662
Proposed branch: lp:~hodgestar/software-properties/configurable-key-server
Merge into: lp:software-properties
Diff against target: 144 lines (+24/-5)
5 files modified
add-apt-repository (+8/-2)
software-properties-gtk (+4/-0)
software-properties-kde (+5/-0)
softwareproperties/SoftwareProperties.py (+1/-1)
softwareproperties/ppa.py (+6/-2)
To merge this branch: bzr merge lp:~hodgestar/software-properties/configurable-key-server
Reviewer Review Type Date Requested Status
Barry Warsaw (community) code Needs Fixing
Ubuntu Core Development Team Pending
Review via email: mp+57406@code.launchpad.net

Description of the change

A small patch that adds a --keyserver option to apt-add-repository. My main use case is so that I can fall back to other alternatives if the Ubuntu key server is down or inaccessible for some reason. I consider the patch just a draft -- I don't know enough about how the code base is used to tell whether I've added the new option to the appropriate places.

To post a comment you must log in.
lp:~hodgestar/software-properties/configurable-key-server updated
659. By Jonathan Riddell

data/designer/main.ui make strings match GTK UI file LP: #760825

660. By Stéphane Graber

In Sources tab, ignore deb-src templates

661. By Stéphane Graber

releasing version 0.80.9

Revision history for this message
Barry Warsaw (barry) wrote :

Hi Simon, thanks very much for the contribution to Ubuntu! I agree this could be a useful addition to the code. It doesn't happen often (these days, IME) but I have seen failures from the ubuntu keyserver, so in those cases it would be nice to have a fallback.

I have some comments on your patch. You're not too far off, but with a few changes, I think it would be a solid addition.

In add-apt-repository, I don't think you need the 'dest' argument to add_option(). The long version of the switch will be taken as the default dest. Also note the misspelling of 'Default' in the help string.

It looks like you have multiple places where 'hkp://keyserver.ubuntu.com:80/' is hardcoded. I'd refactor this so that it's defined exactly once, in a module global constant (upper-cased) in ppa.py. Then in apt-add-repository, I'd change the import at line 9 to be:

    from softwareproperties.ppa import DEFAULT_KEYSERVER, expand_ppa_line

(i.e. DEFAULT_KEYSERVER being the module global). Use that variable in the add_option() call.

Interestingly enough, the SoftwareProperties class already takes an `options` keyword in its __init__(), so I don't think you need to extend that method's signature to take a keyserver argument. In add-apt-repository, just create the instance like this:

    sp = SoftwareProperties(options=options)

I guess the only concern here is whether the `options` that SoftwareProperties takes is intended to be the command line options or something else. Since it's undocumented and untested, let's assume 'yes' although the use of self.options.massive_debug has me scratching my head. Anyway, I'd just pass in options to the constructor.

Now when you instantiate AddPPASigningKeyThread, pass in something like this:

    worker = AddPPASigningKeyThread(parsed_uri.path, options and options.keyserver)

Now in ppa.py, where you add a keyserver argument to AddPPASigningKeyThread.__init__(), use a default value of None instead of the hardcoded path, and do something like this in the __init__():

    self.keyserver = (keyserver if keyserver is not None else DEFAULT_KEYSERVER)

I hope that all makes sense!

I wish this were testable, because then I'd be sure to request that you add a test for your new code :). If you feel like giving it a shot, go for it, but as the rest of the code isn't well tested, I wouldn't block your branch on that.

Marking as needs-fixing for now, but I'm happy to review an update if you push it.

review: Needs Fixing (code)
lp:~hodgestar/software-properties/configurable-key-server updated
663. By Simon Cross <email address hidden>

Factor out default keyserver to a module global. Fix typo in --keyserver docstring. Allow passing in None for the keyserver to fallback to the default value.

664. By Simon Cross <email address hidden>

Move --keyserver into the generic 'options' holder used by SoftwareProperties. This also adds --massive-debug to add-apt-repository and --keyserver to software-properties-gtk and -kde to ensure that all scripts that use SoftwareProperties support the options it needs.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'add-apt-repository'
2--- add-apt-repository 2010-12-07 10:20:45 +0000
3+++ add-apt-repository 2011-05-02 22:01:28 +0000
4@@ -6,7 +6,7 @@
5 import locale
6
7 from softwareproperties.SoftwareProperties import SoftwareProperties
8-from softwareproperties.ppa import expand_ppa_line
9+from softwareproperties.ppa import DEFAULT_KEYSERVER, expand_ppa_line
10 from aptsources.sourceslist import SourceEntry
11 from optparse import OptionParser
12 from gettext import gettext as _
13@@ -41,9 +41,15 @@
14 parser = OptionParser(usage)
15 # FIXME: provide a --sources-list-file= option that
16 # puts the line into a specific file in sources.list.d
17+ parser.add_option ("-m", "--massive-debug", action="store_true",
18+ dest="massive_debug", default=False,
19+ help="Print a lot of debug information to the command line")
20 parser.add_option("-r", "--remove", action="store_true",
21 dest="remove", default=False,
22 help="remove repository from sources.list.d directory")
23+ parser.add_option("-k", "--keyserver",
24+ dest="keyserver", default=DEFAULT_KEYSERVER,
25+ help="URL of keyserver. Default: %default")
26 (options, args) = parser.parse_args()
27
28 if os.geteuid() != 0:
29@@ -57,7 +63,7 @@
30 # force new ppa file to be 644 (LP: #399709)
31 os.umask(0022)
32
33- sp = SoftwareProperties()
34+ sp = SoftwareProperties(options=options)
35 line = args[0]
36 if options.remove:
37 (line, file) = expand_ppa_line(line.strip(), sp.distro.codename)
38
39=== modified file 'software-properties-gtk'
40--- software-properties-gtk 2011-04-05 13:07:35 +0000
41+++ software-properties-gtk 2011-05-02 22:01:28 +0000
42@@ -39,6 +39,7 @@
43 #sys.path.append("@prefix@/share/update-manager/python")
44
45 from softwareproperties.gtk.SoftwarePropertiesGtk import SoftwarePropertiesGtk
46+from softwareproperties.ppa import DEFAULT_KEYSERVER
47
48 if __name__ == "__main__":
49 _ = gettext.gettext
50@@ -70,6 +71,9 @@
51 parser.add_option("--enable-ppa", "",
52 action="store", type="string", default=None,
53 help="Enable PPA with the given name")
54+ parser.add_option("-k", "--keyserver",
55+ dest="keyserver", default=DEFAULT_KEYSERVER,
56+ help="URL of keyserver. Default: %default")
57 parser.add_option("--data-dir", "",
58 action="store", type="string", default="/usr/share/software-properties/",
59 help="Use data files (UI) from the given directory")
60
61=== modified file 'software-properties-kde'
62--- software-properties-kde 2009-12-09 13:21:36 +0000
63+++ software-properties-kde 2011-05-02 22:01:28 +0000
64@@ -32,6 +32,7 @@
65 #sys.path.append("@prefix@/share/update-manager/python")
66
67 from softwareproperties.kde.SoftwarePropertiesKDE import SoftwarePropertiesKDE
68+from softwareproperties.ppa import DEFAULT_KEYSERVER
69
70 from PyKDE4.kdecore import ki18n, KAboutData, KCmdLineArgs , KCmdLineOptions
71 from PyKDE4.kdeui import KApplication, KMessageBox
72@@ -41,6 +42,7 @@
73 massive_debug = False
74 no_update = False
75 enable_component = ""
76+ keyserver = DEFAULT_KEYSERVER
77
78 #--------------- main ------------------
79 if __name__ == '__main__':
80@@ -69,6 +71,7 @@
81 opts.add ("dont-update", ki18n("No update on repository change (useful if called from an external program)"))
82 opts.add ("enable-component <name>", ki18n("Enable the specified component of the distro's repositories"), "component_arg")
83 opts.add ("enable-ppa <name>", ki18n("Enable PPA with the given name"), "ppa_arg")
84+ opts.add ("keyserver <url>", ki18n("URL of keyserver"), "keyserver_url")
85 opts.add ("attach <WinID>", ki18n("Win ID to act as a dialogue for"), "attach_arg")
86
87 KCmdLineArgs.addCmdLineOptions(opts)
88@@ -104,6 +107,8 @@
89 options.no_update = True
90 if args.isSet("attach") == True:
91 attachWinID = args.getOption("attach")
92+ if args.isSet("keyserver"):
93+ options.keyserver = str(args.getOption("keyserver"))
94
95 if args.isSet("enable-ppa"):
96 app = SoftwarePropertiesKDE(datadir=data_dir, options=options, file=file)
97
98=== modified file 'softwareproperties/SoftwareProperties.py'
99--- softwareproperties/SoftwareProperties.py 2010-12-07 10:20:45 +0000
100+++ softwareproperties/SoftwareProperties.py 2011-05-02 22:01:28 +0000
101@@ -616,7 +616,7 @@
102 # FIXME: abstract this all alway into the ppa.py file
103 parsed_uri = urlparse(SourceEntry(srcline).uri)
104 if parsed_uri.netloc == 'ppa.launchpad.net':
105- worker = AddPPASigningKeyThread(parsed_uri.path)
106+ worker = AddPPASigningKeyThread(parsed_uri.path, self.options and self.options.keyserver)
107 worker.start()
108
109 def update_interface(self):
110
111=== modified file 'softwareproperties/ppa.py'
112--- softwareproperties/ppa.py 2010-12-13 08:42:40 +0000
113+++ softwareproperties/ppa.py 2011-05-02 22:01:28 +0000
114@@ -27,6 +27,8 @@
115 from urllib2 import urlopen, Request, URLError
116 from urlparse import urlparse
117
118+DEFAULT_KEYSERVER = "hkp://keyserver.ubuntu.com:80/"
119+
120 def encode(s):
121 return re.sub("[^a-zA-Z0-9_-]","_", s)
122
123@@ -55,9 +57,11 @@
124 class AddPPASigningKeyThread(Thread):
125 " thread class for adding the signing key in the background "
126
127- def __init__(self, ppa_path):
128+ def __init__(self, ppa_path, keyserver=None):
129 Thread.__init__(self)
130 self.ppa_path = ppa_path
131+ self.keyserver = (keyserver if keyserver is not None
132+ else DEFAULT_KEYSERVER)
133
134 def run(self):
135 self.add_ppa_signing_key(self.ppa_path)
136@@ -86,7 +90,7 @@
137 # the load
138 res = subprocess.call(
139 ["apt-key", "adv",
140- "--keyserver", "hkp://keyserver.ubuntu.com:80/",
141+ "--keyserver", self.keyserver,
142 "--recv", signing_key_fingerprint[0]])
143 return (res == 0)
144 except URLError, e:

Subscribers

People subscribed via source and target branches

to status/vote changes: