Status: | Merged |
---|---|
Approved by: | Jelmer Vernooij |
Approved revision: | no longer in the source branch. |
Merge reported by: | The Breezy Bot |
Merged at revision: | not available |
Proposed branch: | lp:~jelmer/brz/ssh-config |
Merge into: | lp:brz |
Diff against target: |
258 lines (+73/-45) 4 files modified
breezy/config.py (+4/-0) breezy/tests/test_ssh_transport.py (+61/-33) breezy/transport/ssh.py (+5/-12) doc/en/release-notes/brz-3.1.txt (+3/-0) |
To merge this branch: | bzr merge lp:~jelmer/brz/ssh-config |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Martin Packman | Approve | ||
Review via email: mp+369320@code.launchpad.net |
Commit message
Allow the ssh vendor to be configured in the config, rather than just using the BRZ_SSH environment variable.
Description of the change
Allow the ssh vendor to be configured in the config, rather than just using the BRZ_SSH environment variable.
To post a comment you must log in.
Revision history for this message
The Breezy Bot (the-breezy-bot) wrote : | # |
Merging failed
https:/
Revision history for this message
The Breezy Bot (the-breezy-bot) wrote : | # |
Merging failed
https:/
Preview Diff
[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1 | === modified file 'breezy/config.py' | |||
2 | --- breezy/config.py 2019-07-07 18:18:20 +0000 | |||
3 | +++ breezy/config.py 2019-07-07 18:58:24 +0000 | |||
4 | @@ -2670,6 +2670,10 @@ | |||
5 | 2670 | help="If we wait for a new request from a client for more than" | 2670 | help="If we wait for a new request from a client for more than" |
6 | 2671 | " X seconds, consider the client idle, and hangup.")) | 2671 | " X seconds, consider the client idle, and hangup.")) |
7 | 2672 | option_registry.register( | 2672 | option_registry.register( |
8 | 2673 | Option('ssh', | ||
9 | 2674 | default=None, override_from_env=['BRZ_SSH'], | ||
10 | 2675 | help='SSH vendor to use.')) | ||
11 | 2676 | option_registry.register( | ||
12 | 2673 | Option('stacked_on_location', | 2677 | Option('stacked_on_location', |
13 | 2674 | default=None, | 2678 | default=None, |
14 | 2675 | help="""The location where this branch is stacked on.""")) | 2679 | help="""The location where this branch is stacked on.""")) |
15 | 2676 | 2680 | ||
16 | === modified file 'breezy/tests/test_ssh_transport.py' | |||
17 | --- breezy/tests/test_ssh_transport.py 2018-11-11 04:08:32 +0000 | |||
18 | +++ breezy/tests/test_ssh_transport.py 2019-07-07 18:58:24 +0000 | |||
19 | @@ -14,7 +14,8 @@ | |||
20 | 14 | # along with this program; if not, write to the Free Software | 14 | # along with this program; if not, write to the Free Software |
21 | 15 | # Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA | 15 | # Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA |
22 | 16 | 16 | ||
24 | 17 | from breezy.tests import TestCase | 17 | from breezy import config |
25 | 18 | from breezy.tests import TestCase, TestCaseWithTransport | ||
26 | 18 | from breezy.errors import SSHVendorNotFound, UnknownSSH | 19 | from breezy.errors import SSHVendorNotFound, UnknownSSH |
27 | 19 | from breezy.transport.ssh import ( | 20 | from breezy.transport.ssh import ( |
28 | 20 | OpenSSHSubprocessVendor, | 21 | OpenSSHSubprocessVendor, |
29 | @@ -37,73 +38,95 @@ | |||
30 | 37 | return self._ssh_version_string | 38 | return self._ssh_version_string |
31 | 38 | 39 | ||
32 | 39 | 40 | ||
34 | 40 | class SSHVendorManagerTests(TestCase): | 41 | class SSHVendorManagerTests(TestCaseWithTransport): |
35 | 41 | 42 | ||
36 | 42 | def test_register_vendor(self): | 43 | def test_register_vendor(self): |
37 | 43 | manager = TestSSHVendorManager() | 44 | manager = TestSSHVendorManager() |
39 | 44 | self.assertRaises(SSHVendorNotFound, manager.get_vendor, {}) | 45 | self.overrideEnv('BRZ_SSH', None) |
40 | 46 | self.assertRaises(SSHVendorNotFound, manager.get_vendor) | ||
41 | 45 | vendor = object() | 47 | vendor = object() |
42 | 46 | manager.register_vendor("vendor", vendor) | 48 | manager.register_vendor("vendor", vendor) |
44 | 47 | self.assertIs(manager.get_vendor({"BRZ_SSH": "vendor"}), vendor) | 49 | self.overrideEnv('BRZ_SSH', 'vendor') |
45 | 50 | self.assertIs(manager.get_vendor(), vendor) | ||
46 | 48 | 51 | ||
47 | 49 | def test_default_vendor(self): | 52 | def test_default_vendor(self): |
48 | 50 | manager = TestSSHVendorManager() | 53 | manager = TestSSHVendorManager() |
50 | 51 | self.assertRaises(SSHVendorNotFound, manager.get_vendor, {}) | 54 | self.overrideEnv('BRZ_SSH', None) |
51 | 55 | self.assertRaises(SSHVendorNotFound, manager.get_vendor) | ||
52 | 52 | vendor = object() | 56 | vendor = object() |
53 | 53 | manager.register_default_vendor(vendor) | 57 | manager.register_default_vendor(vendor) |
55 | 54 | self.assertIs(manager.get_vendor({}), vendor) | 58 | self.assertIs(manager.get_vendor(), vendor) |
56 | 55 | 59 | ||
57 | 56 | def test_get_vendor_by_environment(self): | 60 | def test_get_vendor_by_environment(self): |
58 | 57 | manager = TestSSHVendorManager() | 61 | manager = TestSSHVendorManager() |
65 | 58 | self.assertRaises(SSHVendorNotFound, manager.get_vendor, {}) | 62 | self.overrideEnv('BRZ_SSH', None) |
66 | 59 | self.assertRaises(UnknownSSH, | 63 | self.assertRaises(SSHVendorNotFound, manager.get_vendor) |
67 | 60 | manager.get_vendor, {"BRZ_SSH": "vendor"}) | 64 | self.overrideEnv('BRZ_SSH', 'vendor') |
68 | 61 | vendor = object() | 65 | self.assertRaises(UnknownSSH, manager.get_vendor) |
69 | 62 | manager.register_vendor("vendor", vendor) | 66 | vendor = object() |
70 | 63 | self.assertIs(manager.get_vendor({"BRZ_SSH": "vendor"}), vendor) | 67 | manager.register_vendor("vendor", vendor) |
71 | 68 | self.assertIs(manager.get_vendor(), vendor) | ||
72 | 69 | |||
73 | 70 | def test_get_vendor_by_config(self): | ||
74 | 71 | manager = TestSSHVendorManager() | ||
75 | 72 | self.overrideEnv('BRZ_SSH', None) | ||
76 | 73 | self.assertRaises(SSHVendorNotFound, manager.get_vendor) | ||
77 | 74 | config.GlobalStack().set('ssh', 'vendor') | ||
78 | 75 | self.assertRaises(UnknownSSH, manager.get_vendor) | ||
79 | 76 | vendor = object() | ||
80 | 77 | manager.register_vendor("vendor", vendor) | ||
81 | 78 | self.assertIs(manager.get_vendor(), vendor) | ||
82 | 64 | 79 | ||
83 | 65 | def test_get_vendor_by_inspection_openssh(self): | 80 | def test_get_vendor_by_inspection_openssh(self): |
84 | 66 | manager = TestSSHVendorManager() | 81 | manager = TestSSHVendorManager() |
86 | 67 | self.assertRaises(SSHVendorNotFound, manager.get_vendor, {}) | 82 | self.overrideEnv('BRZ_SSH', None) |
87 | 83 | self.assertRaises(SSHVendorNotFound, manager.get_vendor) | ||
88 | 68 | manager.set_ssh_version_string("OpenSSH") | 84 | manager.set_ssh_version_string("OpenSSH") |
90 | 69 | self.assertIsInstance(manager.get_vendor({}), OpenSSHSubprocessVendor) | 85 | self.assertIsInstance(manager.get_vendor(), OpenSSHSubprocessVendor) |
91 | 70 | 86 | ||
92 | 71 | def test_get_vendor_by_inspection_sshcorp(self): | 87 | def test_get_vendor_by_inspection_sshcorp(self): |
93 | 72 | manager = TestSSHVendorManager() | 88 | manager = TestSSHVendorManager() |
95 | 73 | self.assertRaises(SSHVendorNotFound, manager.get_vendor, {}) | 89 | self.overrideEnv('BRZ_SSH', None) |
96 | 90 | self.assertRaises(SSHVendorNotFound, manager.get_vendor) | ||
97 | 74 | manager.set_ssh_version_string("SSH Secure Shell") | 91 | manager.set_ssh_version_string("SSH Secure Shell") |
99 | 75 | self.assertIsInstance(manager.get_vendor({}), SSHCorpSubprocessVendor) | 92 | self.assertIsInstance(manager.get_vendor(), SSHCorpSubprocessVendor) |
100 | 76 | 93 | ||
101 | 77 | def test_get_vendor_by_inspection_lsh(self): | 94 | def test_get_vendor_by_inspection_lsh(self): |
102 | 78 | manager = TestSSHVendorManager() | 95 | manager = TestSSHVendorManager() |
104 | 79 | self.assertRaises(SSHVendorNotFound, manager.get_vendor, {}) | 96 | self.overrideEnv('BRZ_SSH', None) |
105 | 97 | self.assertRaises(SSHVendorNotFound, manager.get_vendor) | ||
106 | 80 | manager.set_ssh_version_string("lsh") | 98 | manager.set_ssh_version_string("lsh") |
108 | 81 | self.assertIsInstance(manager.get_vendor({}), LSHSubprocessVendor) | 99 | self.assertIsInstance(manager.get_vendor(), LSHSubprocessVendor) |
109 | 82 | 100 | ||
110 | 83 | def test_get_vendor_by_inspection_plink(self): | 101 | def test_get_vendor_by_inspection_plink(self): |
111 | 84 | manager = TestSSHVendorManager() | 102 | manager = TestSSHVendorManager() |
113 | 85 | self.assertRaises(SSHVendorNotFound, manager.get_vendor, {}) | 103 | self.overrideEnv('BRZ_SSH', None) |
114 | 104 | self.assertRaises(SSHVendorNotFound, manager.get_vendor) | ||
115 | 86 | manager.set_ssh_version_string("plink") | 105 | manager.set_ssh_version_string("plink") |
116 | 87 | # Auto-detect of plink vendor disabled, on Windows recommended | 106 | # Auto-detect of plink vendor disabled, on Windows recommended |
117 | 88 | # default ssh-client is paramiko | 107 | # default ssh-client is paramiko |
118 | 89 | # see https://bugs.launchpad.net/bugs/414743 | 108 | # see https://bugs.launchpad.net/bugs/414743 |
121 | 90 | #~self.assertIsInstance(manager.get_vendor({}), PLinkSubprocessVendor) | 109 | #~self.assertIsInstance(manager.get_vendor(), PLinkSubprocessVendor) |
122 | 91 | self.assertRaises(SSHVendorNotFound, manager.get_vendor, {}) | 110 | self.assertRaises(SSHVendorNotFound, manager.get_vendor) |
123 | 92 | 111 | ||
124 | 93 | def test_cached_vendor(self): | 112 | def test_cached_vendor(self): |
125 | 94 | manager = TestSSHVendorManager() | 113 | manager = TestSSHVendorManager() |
127 | 95 | self.assertRaises(SSHVendorNotFound, manager.get_vendor, {}) | 114 | self.overrideEnv('BRZ_SSH', None) |
128 | 115 | self.assertRaises(SSHVendorNotFound, manager.get_vendor) | ||
129 | 96 | vendor = object() | 116 | vendor = object() |
130 | 97 | manager.register_vendor("vendor", vendor) | 117 | manager.register_vendor("vendor", vendor) |
132 | 98 | self.assertRaises(SSHVendorNotFound, manager.get_vendor, {}) | 118 | self.assertRaises(SSHVendorNotFound, manager.get_vendor) |
133 | 99 | # Once the vendor is found the result is cached (mainly because of the | 119 | # Once the vendor is found the result is cached (mainly because of the |
134 | 100 | # 'get_vendor' sometimes can be an expensive operation) and later | 120 | # 'get_vendor' sometimes can be an expensive operation) and later |
135 | 101 | # invocations of the 'get_vendor' just returns the cached value. | 121 | # invocations of the 'get_vendor' just returns the cached value. |
138 | 102 | self.assertIs(manager.get_vendor({"BRZ_SSH": "vendor"}), vendor) | 122 | self.overrideEnv('BRZ_SSH', 'vendor') |
139 | 103 | self.assertIs(manager.get_vendor({}), vendor) | 123 | self.assertIs(manager.get_vendor(), vendor) |
140 | 124 | self.overrideEnv('BRZ_SSH', None) | ||
141 | 125 | self.assertIs(manager.get_vendor(), vendor) | ||
142 | 104 | # The cache can be cleared by the 'clear_cache' method | 126 | # The cache can be cleared by the 'clear_cache' method |
143 | 105 | manager.clear_cache() | 127 | manager.clear_cache() |
145 | 106 | self.assertRaises(SSHVendorNotFound, manager.get_vendor, {}) | 128 | self.overrideEnv('BRZ_SSH', None) |
146 | 129 | self.assertRaises(SSHVendorNotFound, manager.get_vendor) | ||
147 | 107 | 130 | ||
148 | 108 | def test_get_vendor_search_order(self): | 131 | def test_get_vendor_search_order(self): |
149 | 109 | # The 'get_vendor' method search for SSH vendors as following: | 132 | # The 'get_vendor' method search for SSH vendors as following: |
150 | @@ -118,33 +141,37 @@ | |||
151 | 118 | 141 | ||
152 | 119 | manager = TestSSHVendorManager() | 142 | manager = TestSSHVendorManager() |
153 | 120 | # At first no vendors are found | 143 | # At first no vendors are found |
155 | 121 | self.assertRaises(SSHVendorNotFound, manager.get_vendor, {}) | 144 | self.overrideEnv('BRZ_SSH', None) |
156 | 145 | self.assertRaises(SSHVendorNotFound, manager.get_vendor) | ||
157 | 122 | 146 | ||
158 | 123 | # If the default vendor is registered it will be returned | 147 | # If the default vendor is registered it will be returned |
159 | 124 | default_vendor = object() | 148 | default_vendor = object() |
160 | 125 | manager.register_default_vendor(default_vendor) | 149 | manager.register_default_vendor(default_vendor) |
162 | 126 | self.assertIs(manager.get_vendor({}), default_vendor) | 150 | self.assertIs(manager.get_vendor(), default_vendor) |
163 | 127 | 151 | ||
164 | 128 | # If the known vendor is found in the system it will be returned | 152 | # If the known vendor is found in the system it will be returned |
165 | 129 | manager.clear_cache() | 153 | manager.clear_cache() |
166 | 130 | manager.set_ssh_version_string("OpenSSH") | 154 | manager.set_ssh_version_string("OpenSSH") |
168 | 131 | self.assertIsInstance(manager.get_vendor({}), OpenSSHSubprocessVendor) | 155 | self.assertIsInstance(manager.get_vendor(), OpenSSHSubprocessVendor) |
169 | 132 | 156 | ||
170 | 133 | # If the BRZ_SSH environment variable is found it will be treated as | 157 | # If the BRZ_SSH environment variable is found it will be treated as |
171 | 134 | # the vendor name | 158 | # the vendor name |
172 | 135 | manager.clear_cache() | 159 | manager.clear_cache() |
173 | 136 | vendor = object() | 160 | vendor = object() |
174 | 137 | manager.register_vendor("vendor", vendor) | 161 | manager.register_vendor("vendor", vendor) |
176 | 138 | self.assertIs(manager.get_vendor({"BRZ_SSH": "vendor"}), vendor) | 162 | self.overrideEnv('BRZ_SSH', 'vendor') |
177 | 163 | self.assertIs(manager.get_vendor(), vendor) | ||
178 | 139 | 164 | ||
179 | 140 | # Last cached value always checked first | 165 | # Last cached value always checked first |
181 | 141 | self.assertIs(manager.get_vendor({}), vendor) | 166 | self.overrideEnv('BRZ_SSH', 'vendor') |
182 | 167 | self.assertIs(manager.get_vendor(), vendor) | ||
183 | 142 | 168 | ||
184 | 143 | def test_get_vendor_from_path_win32_plink(self): | 169 | def test_get_vendor_from_path_win32_plink(self): |
185 | 144 | manager = TestSSHVendorManager() | 170 | manager = TestSSHVendorManager() |
186 | 145 | manager.set_ssh_version_string("plink: Release 0.60") | 171 | manager.set_ssh_version_string("plink: Release 0.60") |
187 | 146 | plink_path = "C:/Program Files/PuTTY/plink.exe" | 172 | plink_path = "C:/Program Files/PuTTY/plink.exe" |
189 | 147 | vendor = manager.get_vendor({"BRZ_SSH": plink_path}) | 173 | self.overrideEnv('BRZ_SSH', plink_path) |
190 | 174 | vendor = manager.get_vendor() | ||
191 | 148 | self.assertIsInstance(vendor, PLinkSubprocessVendor) | 175 | self.assertIsInstance(vendor, PLinkSubprocessVendor) |
192 | 149 | args = vendor._get_vendor_specific_argv("user", "host", 22, ["bzr"]) | 176 | args = vendor._get_vendor_specific_argv("user", "host", 22, ["bzr"]) |
193 | 150 | self.assertEqual(args[0], plink_path) | 177 | self.assertEqual(args[0], plink_path) |
194 | @@ -154,7 +181,8 @@ | |||
195 | 154 | manager.set_ssh_version_string( | 181 | manager.set_ssh_version_string( |
196 | 155 | "OpenSSH_5.1p1 Debian-5, OpenSSL, 0.9.8g 19 Oct 2007") | 182 | "OpenSSH_5.1p1 Debian-5, OpenSSL, 0.9.8g 19 Oct 2007") |
197 | 156 | openssh_path = "/usr/bin/ssh" | 183 | openssh_path = "/usr/bin/ssh" |
199 | 157 | vendor = manager.get_vendor({"BRZ_SSH": openssh_path}) | 184 | self.overrideEnv('BRZ_SSH', openssh_path) |
200 | 185 | vendor = manager.get_vendor() | ||
201 | 158 | self.assertIsInstance(vendor, OpenSSHSubprocessVendor) | 186 | self.assertIsInstance(vendor, OpenSSHSubprocessVendor) |
202 | 159 | args = vendor._get_vendor_specific_argv("user", "host", 22, ["bzr"]) | 187 | args = vendor._get_vendor_specific_argv("user", "host", 22, ["bzr"]) |
203 | 160 | self.assertEqual(args[0], openssh_path) | 188 | self.assertEqual(args[0], openssh_path) |
204 | 161 | 189 | ||
205 | === modified file 'breezy/transport/ssh.py' | |||
206 | --- breezy/transport/ssh.py 2019-06-16 01:52:23 +0000 | |||
207 | +++ breezy/transport/ssh.py 2019-07-07 18:58:24 +0000 | |||
208 | @@ -80,16 +80,9 @@ | |||
209 | 80 | """Clear previously cached lookup result.""" | 80 | """Clear previously cached lookup result.""" |
210 | 81 | self._cached_ssh_vendor = None | 81 | self._cached_ssh_vendor = None |
211 | 82 | 82 | ||
222 | 83 | def _get_vendor_by_environment(self, environment=None): | 83 | def _get_vendor_by_config(self): |
223 | 84 | """Return the vendor or None based on BRZ_SSH environment variable. | 84 | vendor_name = config.GlobalStack().get('ssh') |
224 | 85 | 85 | if vendor_name is not None: | |
215 | 86 | :raises UnknownSSH: if the BRZ_SSH environment variable contains | ||
216 | 87 | unknown vendor name | ||
217 | 88 | """ | ||
218 | 89 | if environment is None: | ||
219 | 90 | environment = os.environ | ||
220 | 91 | if 'BRZ_SSH' in environment: | ||
221 | 92 | vendor_name = environment['BRZ_SSH'] | ||
225 | 93 | try: | 86 | try: |
226 | 94 | vendor = self._ssh_vendors[vendor_name] | 87 | vendor = self._ssh_vendors[vendor_name] |
227 | 95 | except KeyError: | 88 | except KeyError: |
228 | @@ -151,7 +144,7 @@ | |||
229 | 151 | return self._get_vendor_by_version_string(version, | 144 | return self._get_vendor_by_version_string(version, |
230 | 152 | os.path.splitext(os.path.basename(path))[0]) | 145 | os.path.splitext(os.path.basename(path))[0]) |
231 | 153 | 146 | ||
233 | 154 | def get_vendor(self, environment=None): | 147 | def get_vendor(self): |
234 | 155 | """Find out what version of SSH is on the system. | 148 | """Find out what version of SSH is on the system. |
235 | 156 | 149 | ||
236 | 157 | :raises SSHVendorNotFound: if no any SSH vendor is found | 150 | :raises SSHVendorNotFound: if no any SSH vendor is found |
237 | @@ -159,7 +152,7 @@ | |||
238 | 159 | unknown vendor name | 152 | unknown vendor name |
239 | 160 | """ | 153 | """ |
240 | 161 | if self._cached_ssh_vendor is None: | 154 | if self._cached_ssh_vendor is None: |
242 | 162 | vendor = self._get_vendor_by_environment(environment) | 155 | vendor = self._get_vendor_by_config() |
243 | 163 | if vendor is None: | 156 | if vendor is None: |
244 | 164 | vendor = self._get_vendor_by_inspection() | 157 | vendor = self._get_vendor_by_inspection() |
245 | 165 | if vendor is None: | 158 | if vendor is None: |
246 | 166 | 159 | ||
247 | === modified file 'doc/en/release-notes/brz-3.1.txt' | |||
248 | --- doc/en/release-notes/brz-3.1.txt 2019-07-07 18:18:20 +0000 | |||
249 | +++ doc/en/release-notes/brz-3.1.txt 2019-07-07 18:58:24 +0000 | |||
250 | @@ -54,6 +54,9 @@ | |||
251 | 54 | * Automatically upgrade to branch format 8 when setting branch references. | 54 | * Automatically upgrade to branch format 8 when setting branch references. |
252 | 55 | (Jelmer Vernooij) | 55 | (Jelmer Vernooij) |
253 | 56 | 56 | ||
254 | 57 | * The ``ssh`` configuration variable can be used to set the default | ||
255 | 58 | SSH implementation. (Jelmer Vernooij, #650757) | ||
256 | 59 | |||
257 | 57 | * ``locks.steal_dead`` is now enabled by default. | 60 | * ``locks.steal_dead`` is now enabled by default. |
258 | 58 | (Jelmer Vernooij, #220464) | 61 | (Jelmer Vernooij, #220464) |
259 | 59 | 62 |
Makes sense to have in config.