Opened 3 years ago

Closed 20 months ago

#875 closed defect (fixed)

properties files get unduly modified by installation procedure

Reported by: pbaumann Owned by: bphamhuu
Priority: minor Milestone: 9.1.x
Component: build system Version: development
Keywords: Cc: dmisev
Complexity: Medium

Description

the "make install" script for handling the petascope.properties and log4j.properties file can create a new file. While it attempts to copy over old settings some stuff, like comments, may get lost. The preocedure is meant for getting in new keys/values, which per se is good. A better approach, however, is to keep the old file and just append any newly introduced variables.

PS: in the script, this text is unclear:

"service existing service file, will not modify it"

Attachments (2)

Change History (14)

comment:1 Changed 21 months ago by dmisev

  • Component changed from undecided to autotools
  • Milestone changed from Future to 9.1.x
  • Owner changed from pbaumann to bphamhuu
  • Status changed from new to assigned

Bang, please make sure the existing petascope.properties file is backed up before it's modified.

comment:2 Changed 21 months ago by bphamhuu

Dimitar: agree (I can see you make backup petascope.properties before 9 in make install), but I don't know what is called "new" variable in here (new with one but old with others) (so the best way is compare with user's file, if they don't have property then I will add). I hope understand correctly (in fact, I also did it with Jetty properties (start_embedded_petascope and java_server)).

comment:3 Changed 21 months ago by dmisev

As discussed today, it's probably best to take that logic for updating the configuration file out of the Makefile.am, and put it in a script which will give us more power (e.g. python or bash).

comment:4 Changed 21 months ago by bphamhuu

ok, I got it, user will configured everything in petascope.properties (log4j.properties) in Rasdaman source code first and my script will get old configuration from user and modify the new petascope.properties last time before overwrite (with backup) to $RMANHOME/etc.

comment:5 Changed 20 months ago by bphamhuu

  • Resolution set to fixed
  • Status changed from assigned to closed

The patch has been submitted as it has been validated carefully and accepted both by Dimitar, I will close this ticket here.

comment:6 Changed 20 months ago by dmisev

  • Resolution fixed deleted
  • Status changed from closed to reopened

There is a slight bug in the update_properties.sh script: it creates petascope.properties backups on every make install. So I end up with a bunch of petascope.properties*.bak in $RMANHOME/etc, even though the petascope.properties file has not changed at all.

No backup should be left in etc/ if nothing has been updated.

Also the permissions of the petascope.properties are not right, they should be same like the other config files (-rw-r--r--):

-rw-r--r-- 1 dimitar dimitar  711 Nov 29 09:18 log-server.conf
-rwxr-xr-x 1 dimitar dimitar 7441 Dec  2 17:37 petascope.properties
-rwxr-xr-x 1 dimitar dimitar 7441 Dec  2 17:37 petascope.properties.12-02-2015_17-37-14.bak

comment:7 Changed 20 months ago by bphamhuu

Ok, Dimitar, actually a lot of things has added from the original purpose so I did not take care to check both file are identical first. A patch has been submitted and also fix the permission to 644 as you want. When the patch is accepted I will close this again.

comment:8 Changed 20 months ago by dmisev

Thanks for the quick fix! Please submit for review first next time, I had to go to the patchmanager to see the code.

+#2.3 Check if NEW and OLD file are the same (no need to create a backup and do anything)
+logn "Checking the difference between Old and New configuration files..."
+diff=$(diff "$NEW" "$OLD")
+if [[ $diff == "" ]]; then
+   echo "Your old configuration is already up to date."
+   ok
+fi
+echo "Done."

$diff should be in quotes in the if statement; "Checking the difference bet..." is not needed log output. It's better like this:

+#2.3 Check if NEW and OLD file are the same (no need to create a backup and do anything)
+cmp --quiet "$NEW" "$OLD"
+if [ $? -eq 0 ]; then
+   log "The existing configuration is already up to date."
+   ok
+fi

comment:9 Changed 20 months ago by bphamhuu

Sorry for inconvenience as I thought this is too small so don't push to "codereview", so I upload the patch manager to here (please check, if it is passed then I will upload to patch manager tomorrow - so you can delete the earlier patch before. Thanks).

http://rasdaman.org/attachment/ticket/875/0001-ticket-875-properties-files-get-unduly-modified-by-i.patch

comment:10 Changed 20 months ago by dmisev

Ok Bang, please test your fix first, it doesn't look like you did that at all. And remove the

logn "Checking the difference between Old and New configuration files..."

No rush, it doesn't need to be done right now in the night of course.

Last edited 20 months ago by dmisev (previous) (diff)

comment:11 Changed 20 months ago by bphamhuu

Hi Dimitar,

it should not be rush of course, but I also run update_petascope.properties few times before submitting a patch (and work in good behavior). I could not get your idea about it did not work as you want (it just compare Old and New configuration and if it is the same then exit NO_ERROR - do you want to add something for more detail?).

I submit a new patch without the line you don't want to see and of course run with "make install" inside petascope directory few times alo, please download and recheck.

http://www.rasdaman.org/attachment/ticket/875/0001-ticket-875-properties-files-get-unduly-modified-by.patch

comment:12 Changed 20 months ago by bphamhuu

  • Resolution set to fixed
  • Status changed from reopened to closed

I will close as Dimitar has accepted and allow me to submit a patch. Thanks.

Note: See TracTickets for help on using tickets.