Simone Tiraboschi has posted comments on this change. Change subject: WebSocketProxy on a separate host ......................................................................
Patch Set 24: (11 comments) http://gerrit.ovirt.org/#/c/26898/24/packaging/setup/ovirt_engine_setup/engineconstants.py File packaging/setup/ovirt_engine_setup/engineconstants.py: Line 1: # Line 2: # ovirt-engine-setup -- ovirt engine setup Line 3: # Copyright (C) 2013 Red Hat, Inc. > 2014 Done Line 4: # Line 5: # Licensed under the Apache License, Version 2.0 (the "License"); Line 6: # you may not use this file except in compliance with the License. Line 7: # You may obtain a copy of the License at http://gerrit.ovirt.org/#/c/26898/24/packaging/setup/plugins/ovirt-engine-setup/ovirt-engine/config/options.py File packaging/setup/plugins/ovirt-engine-setup/ovirt-engine/config/options.py: Line 102: ]: Line 103: self.logger.error( Line 104: _('Engine configuration rejected with engine ' Line 105: 'rpms in place') Line 106: ) > please use the same indentation/style used in the following RuntimeError Done Line 107: raise RuntimeError( Line 108: _( Line 109: 'Engine Configuration was rejected by user: ' Line 110: 'if you don\'t need it, please remove engine rpms' http://gerrit.ovirt.org/#/c/26898/24/packaging/setup/plugins/ovirt-engine-setup/websocket_proxy/config.py File packaging/setup/plugins/ovirt-engine-setup/websocket_proxy/config.py: Line 218: ] Line 219: Line 220: if self._enabled: Line 221: if self._local_engine: Line 222: # Local Engine ... > above comment is useless Done Line 223: if self.environment[ Line 224: osetupcons.ConfigEnv.WEBSOCKET_PROXY_CONFIG_LOCAL Line 225: ] is None: Line 226: self.environment[ Line 258: if self._local_wsp: Line 259: self.environment[ Line 260: osetupcons.ConfigEnv.WEBSOCKET_PROXY_HOST Line 261: ] = "Engine" Line 262: # ] = self.environment[osetupcons.ConfigEnv.FQDN] > please remove above comment Done Line 263: else: Line 264: if self._wsp_host is None: Line 265: fqdn = socket.getfqdn() Line 266: while True: Line 266: while True: Line 267: self._wsp_host = self.dialog.queryString( Line 268: name='OVESETUP_CONFIG_WEBSOCKET_PROXY_HOST', Line 269: note=_('Host fully qualified DNS name of \n' Line 270: 'WebSocket Proxy host [@DEFAULT@]: '), > note=_( Done Line 271: prompt=True, Line 272: default=fqdn, Line 273: ) Line 274: try: Line 272: default=fqdn, Line 273: ) Line 274: try: Line 275: socket.getaddrinfo(self._wsp_host, None) Line 276: break # do while missing in python > you can use a variable assigning it here and loop over variable is None in Done Line 277: except socket.error as e: Line 278: self.logger.error( Line 279: _('Host is invalid: {error}').format( Line 280: error=e.strerror Line 415: fqdn = self.environment[osetupcons.ConfigEnv.FQDN] Line 416: else: Line 417: fqdn = self.environment[ Line 418: osetupcons.ConfigEnv.WEBSOCKET_PROXY_HOST Line 419: ] > I may be wrong but above may be: Done Line 420: Line 421: self.execute( Line 422: args=( Line 423: osetupcons.FileLocations.OVIRT_ENGINE_PKI_CA_ENROLL, Line 488: 'engine host? ' Line 489: '(@VALUES@)[@DEFAULT@]: ' Line 490: ), Line 491: prompt=True, Line 492: validValues=(_('Yes'), _('No')), > I'm pretty sure that a queryBoolean method is available for this kind of ye Done Line 493: caseSensitive=False, Line 494: default=_('Yes') Line 495: ) == _('Yes').lower() Line 496: Line 629: OVIRT_ENGINE_PKI_ENGINE_CERT Line 630: ) Line 631: finally: Line 632: sftp.close() Line 633: except paramiko.AuthenticationException: > self.logger.debug('exception', exc_info=True) Done Line 634: self.logger.error( Line 635: _('Invalid password for host {fqdn}').format( Line 636: fqdn=remote_engine_fqdn, Line 637: ) Line 661: ) Line 662: ) Line 663: except IOError as e: Line 664: if e.errno == errno.ENOENT: Line 665: self.logger.debug('exception', exc_info=True) > please move above out of the if, we do the same in the else branch. I prefer to keep the two case separated cause in the first case the file is not present on the remote host (probably the cert has not been generated there) and so on my opinion we can help the user to quickly point it out giving him an hint, in the second case everything else. Line 666: self.logger.error( Line 667: _( Line 668: 'Unable to connect to fetch file {file} from ' Line 669: ' {host}: file not present' Line 681: 'are they been generated on the engine host?' Line 682: ) Line 683: ) Line 684: else: Line 685: self.logger.debug('exception', exc_info=True) > see above Done Line 686: self.logger.error( Line 687: _( Line 688: 'Unable to connect to fetch file {file} from ' Line 689: ' {host}' -- To view, visit http://gerrit.ovirt.org/26898 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I169604429e5a2d72573b05c0e5481306edfdd935 Gerrit-PatchSet: 24 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Simone Tiraboschi <stira...@redhat.com> Gerrit-Reviewer: Lev Veyde <lve...@gmail.com> Gerrit-Reviewer: Sandro Bonazzola <sbona...@redhat.com> Gerrit-Reviewer: Simone Tiraboschi <stira...@redhat.com> Gerrit-Reviewer: Yedidyah Bar David <d...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
