Martin Peřina has posted comments on this change. Change subject: kdump: Add kdump plugin ......................................................................
Patch Set 4: (11 comments) http://gerrit.ovirt.org/#/c/28385/4/src/plugins/ovirt-host-deploy/Makefile.am File src/plugins/ovirt-host-deploy/Makefile.am: Line 28: tune \ Line 29: node \ Line 30: gluster \ Line 31: openstack \ Line 32: kdump \ > tab please Done Line 33: $(NULL) Line 34: Line 35: install-data-local: Line 36: $(MKDIR_P) "$(DESTDIR)$(otopiplugindir)" http://gerrit.ovirt.org/#/c/28385/4/src/plugins/ovirt-host-deploy/core/offlinepackager.py File src/plugins/ovirt-host-deploy/core/offlinepackager.py: Line 76: Line 77: pkgs = [] Line 78: if ( Line 79: name in ('redhat', 'centos') and Line 80: version.split('.')[0] == '6' or > almost sure you need () here I don't need (), and operator has a precedence over or operator. I looked at the doc to be sure, but it's quite common in most languages Line 81: name == 'fedora' and Line 82: version == '20' Line 83: ): Line 84: pkgs = [ Line 78: if ( Line 79: name in ('redhat', 'centos') and Line 80: version.split('.')[0] == '6' or Line 81: name == 'fedora' and Line 82: version == '20' > for fedora the version is important... as >= 20 will be supported. Done Line 83: ): Line 84: pkgs = [ Line 85: { Line 86: 'operation': 'installed', http://gerrit.ovirt.org/#/c/28385/4/src/plugins/ovirt-host-deploy/kdump/packages.py File src/plugins/ovirt-host-deploy/kdump/packages.py: Line 57: _FK_OPTS_REGEX = re.compile( Line 58: flags=re.VERBOSE, Line 59: pattern=r""" Line 60: ^ Line 61: (?P<key>fence_kdump_(nodes|args)) > spaces before are not allowed? AFAIK they are not. Line 62: \s Line 63: (?P<value>.*) Line 64: $ Line 65: """ Line 59: pattern=r""" Line 60: ^ Line 61: (?P<key>fence_kdump_(nodes|args)) Line 62: \s Line 63: (?P<value>.*) > if you go this way you do not need <key> and <value> as you do not use them Done Line 64: $ Line 65: """ Line 66: ) Line 67: Line 111: fk_bck_m = self._FK_BACKUP_REGEX.match(line) Line 112: if fk_opt_m is not None: Line 113: # line with fence_kdump config Line 114: backup.append('#' + line) Line 115: else: > if you use else and if, why don't you move the match here as well? Done Line 116: if fk_bck_m is not None: Line 117: # line with fence_kdump config backup Line 118: backup_exists = True Line 119: Line 120: new_content.append(line) Line 121: Line 122: if not backup_exists and backup: Line 123: new_content.append('#ovirt-host-deploy:backup-begin') Line 124: new_content.extend(backup) > I prefer to have raw lines within backup and here: Done Line 125: new_content.append('#ovirt-host-deploy:backup-end') Line 126: Line 127: new_content.extend( Line 128: ( Line 140: @plugin.event( Line 141: stage=plugin.Stages.STAGE_INIT, Line 142: ) Line 143: def _setup(self): Line 144: self.environment[odeploycons.KdumpEnv.ENABLE] = False > this must be setdefault, so we can use answer file Done Line 145: self.environment[odeploycons.KdumpEnv.SUPPORTED] = False Line 146: Line 147: @plugin.event( Line 148: stage=plugin.Stages.STAGE_CUSTOMIZATION, Line 141: stage=plugin.Stages.STAGE_INIT, Line 142: ) Line 143: def _setup(self): Line 144: self.environment[odeploycons.KdumpEnv.ENABLE] = False Line 145: self.environment[odeploycons.KdumpEnv.SUPPORTED] = False > this can or cannot be. I don't understand. So should this be set using setdefault? Line 146: Line 147: @plugin.event( Line 148: stage=plugin.Stages.STAGE_CUSTOMIZATION, Line 149: priority=plugin.Stages.PRIORITY_HIGH, Line 159: for package in result: Line 160: cur_version = '%s-%s' % ( Line 161: package['version'], Line 162: package['release'], Line 163: ) > you do not actually need this cur_version temp var... but I will ignore thi Yes, but it's much more readable then split line in if :-) Line 164: if LooseVersion(cur_version) >= LooseVersion(min_version): Line 165: self.environment[odeploycons.KdumpEnv.SUPPORTED] = True Line 166: break Line 167: Line 165: self.environment[odeploycons.KdumpEnv.SUPPORTED] = True Line 166: break Line 167: Line 168: # suppress other plugin stages if not supported Line 169: self._enabled = self.environment[odeploycons.KdumpEnv.SUPPORTED] > no need... as it is disabled anyway. Done Line 170: Line 171: @plugin.event( Line 172: stage=plugin.Stages.STAGE_PACKAGES, Line 173: condition=lambda self: self.environment[odeploycons.KdumpEnv.ENABLE], -- To view, visit http://gerrit.ovirt.org/28385 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Idda48cb053c7e8747de5434c3681403f739c06b1 Gerrit-PatchSet: 4 Gerrit-Project: ovirt-host-deploy Gerrit-Branch: master Gerrit-Owner: Martin Peřina <mper...@redhat.com> Gerrit-Reviewer: Alon Bar-Lev <alo...@redhat.com> Gerrit-Reviewer: Barak Azulay <bazu...@redhat.com> Gerrit-Reviewer: Martin Peřina <mper...@redhat.com> Gerrit-Reviewer: Oved Ourfali <oourf...@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
