1. About packaging/package.sh
1.1. I personally dislike the use of bashism, e.g. like reserved keywords,
double '[[', double '((', but that's ok
for targeted distro (RHEL/Centos). The only requirement is please add more
comments in tough places
to improve code readability and maintanbility.
1.2. few code style comments:
>> name=$(cat /etc/*release | grep ^NAME | sed -r 's|.*"(.*)".*|\1|')
add
| head -n 1
or else it will fail in case more than one file with *release mask in /etc
1.3. rework prepEnv
>> if [ ! -z installCmd ]; then
this will fail miserably, replace with
if [ ! -z $installCmd ]; then
1.3.1. however running package.sh under Ubuntu still would fail due to
$installCmd is unpacked to 'apt --no-install-recommends'
and -z 'apt --no-install-recommends' would always return false.
1.3.2. and still, when apt/yum is installed and rpm/rpm-build is not, the
whole prepEnv would fail to ensure all
build-time requirements are installed, because it only checks for apt/yum.
1.3.3. installing requires root privileges. However, no checks for $EUID nor
running apt/yum with 'sudo' present.
Please, add this check, otherwise running package.sh under unprivileged user
with no installed build-time dependencies
will fail with very undescriptive errors.
1.4. getBin
1.4.1. I don't like detecting target apache ignite package version by
parsing embedded changelog. This might fail
when any package text description would contain '*' in the first char of the
string. Either move changelog to separate
Changelog file and %include it in .spec or parse for, e.g. '^Version:
[0-9\.]+'
>> igniteVersion=$(cat rpm/SPECS/apache-ignite.spec | grep '^*' | head -1 |
>> sed -r 's|.*\ -\ (.*)-.*|\1|')
1.4.2.
>> ls ${binName} && binPreparedFlag=true || true
why not use
if [ ! -f $binName ];
1.4.3. 'curl' is used, but not checked as build-time dependency.
1.4.4. we try to find and download 'apache-ignite-fabric-$igniteVersion',
that's ok for <2.4 releases, but in future
we would need to find and download 'apache-ignite-$igniteVersion' too.
Better add this check as a fallback option, e.g.
try to use curl with both urls in correct order.
if curl --output /dev/null --silent --head --fail "$url"; then
echo "URL exists: $url"
else
echo "URL does not exist: $url"
fi
1.4.5. also strange, but may be ok that we download
apache-ignite-fabric-2.4.0
2. about apache-ignite.spec
2.1. fix typo
>> Apache Ignite's optinal libs and integrations
2.2. fix description
>> C++ files necessary for using Apache Ignite
platform files are not necessary for Ignite, only for integration with .NET,
afaik?
2.3. Tough place with 'Obsoletes: apache-ignite < 2.5.0'
I understand the current logic behind splitting apache-ignite to several
.rpm's and setting obsolete for the old
apache-ignite.
However, for future releases, e.g. if/when we handover this task to another
engineer, it is not quite obvious that one
must (or must not?) update version in 5 different places, including
changelog. Please add a few lines about
'How to package next (new) version of Apache Ignite RPMs' to Confluence.
2.4. firewalld / firewall-cmd is optionally used if installed, but not set
in the dependency. It is OK, but may be we
add this as a post-install note, like we do recommendation messages in node
logs.
3. service scripts : we'll should add a test suite for it, overall looks
pretty solid.
--
Sent from: http://apache-ignite-developers.2346864.n4.nabble.com/
