Ofer Schreiber has posted comments on this change.
Change subject: tools: Make system and REST API rebase for log collector
......................................................................
Patch Set 4: Fails
(8 inline comments)
Looks ok, few small issues inside.
....................................................
File Makefile
Line 49: tar --transform 's,^,/$(NAME_VER)/,S' -z -c -f $(TARBALL) `git
ls-files`
We just saw a bug on this.
the usage of `git ls-files` in the make file is problematic.
(think about the case someone takes your .tar.gz without cloning the git)
consider using:
mkdir $(NAME)
rsync -avz --exclude=.git . $(NAME)
tar -cvf $(TARBALL) $(NAME)
-rm -rf $(NAME)/
(not a clear issue though, it's up to you)
Line 56: sed -i -e's/^Name:.*/Name: $(NAME)/' $(SPEC_FILE)
any reason for this sed?
Line 60: rpmbuild -bs --define="_topdir $(RPMTOP)" --define="_sourcedir
." $(SPEC_FILE)
should be --define="_sourcedir $(RPMTOP)/SOURCES"
Line 81: rm -f $(PREFIX)/usr/bin/engine-log-collector
why do you need to remove this file?
....................................................
File ovirt-log-collector.spec.in
Line 4: Source: %{name}-%{version}.tar.gz
iirc, it better be Source0
Line 28: %attr (755,root,root) %{_datadir}/ovirt-engine/log-collector
if the Makefile already puts the right permissions, there's no need for the
"%attr (755, root, root)
In that case, when you'll think you need to change some permission, you will be
abler to change it in one place
....................................................
File src/rhev/engine-log-collector.8
Line 4: engine\-log\-collector \- oVirt Enterprise Virtualization Engine
Manager Log Collector
Should be "oVirt Log Collector"
....................................................
File src/sos/plugins/engine.py
Line 8: ("vdsmlogs", 'Directory containing all of the SOS logs from
the oVirt hypervisor(s)', '', False),
If you ask me, you can drop the oVirt/RHEV string"
"from hypervisor(s)" is clear enough, and you won't have any trouble later in
the process.
--
To view, visit http://gerrit.ovirt.org/3212
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I2648032f587118e2b0a5b3c194c4ba84390b37a2
Gerrit-PatchSet: 4
Gerrit-Project: ovirt-log-collector
Gerrit-Branch: master
Gerrit-Owner: Keith Robertson <[email protected]>
Gerrit-Reviewer: Barak Azulay <[email protected]>
Gerrit-Reviewer: Keith Robertson <[email protected]>
Gerrit-Reviewer: Ofer Schreiber <[email protected]>
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
