Bug 10104

Summary: koha-disable error handling
Product: Koha Reporter: Tomás Cohen Arazi <tomascohen>
Component: PackagingAssignee: Chris Cormack <chris>
Status: CLOSED FIXED QA Contact:
Severity: normal    
Priority: P5 - low CC: chris, mtj, robin
Version: Main   
Hardware: All   
OS: All   
Change sponsored?: Sponsored Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on:    
Bug Blocks: 4876    
Attachments: Bug 10104 - make koha-disable more robust
Signed off patch
Bug 10104 - Followup: fix param check
Follow up signed off
Bug 10104 - make koha-disable more robust
Bug 10104 - Followup: fix param check

Description Tomás Cohen Arazi 2013-04-23 16:18:12 UTC
Make koha-disable handle errors more gracefully. Also make it support more than one instance as parameter (there is a for loop, but a previous check prevents it to work).
Comment 1 Tomás Cohen Arazi 2013-04-23 16:34:50 UTC Comment hidden (obsolete)
Comment 2 Chris Cormack 2013-04-28 19:44:16 UTC
Testing now

On a disabled instance
sudo ./debian/scripts/koha-disable persona           
Instance persona already disabled.

On an unknown instance
sudo ./debian/scripts/koha-disable pers              
Unknown instance pers.

On an enabled instance 
sudo ./debian/scripts/koha-disable persona
[....] Restarting web server: apache2
ok

Looks all good to me, I confirm its disabled.
Comment 3 Chris Cormack 2013-04-28 19:45:05 UTC Comment hidden (obsolete)
Comment 4 Mason James 2013-04-29 03:40:48 UTC
a small QA question, this patch breaks the default 'usage' message, when no arg is passed - is this intended?

-[ "$#" = 1 ] || die "Usage: $0 instancename..."
+[ "$#" > 1 ] || die "Usage: $0 instancename..."


(this is the same issue as bug-10101)
Comment 5 Tomás Cohen Arazi 2013-04-29 18:23:10 UTC Comment hidden (obsolete)
Comment 6 Chris Cormack 2013-04-29 20:01:22 UTC
 sudo ./debian/scripts/koha-disable                   
Disables Koha instances.

Usage: ./debian/scripts/koha-disable instancename1 instancename2...

Missing instance name...
Comment 7 Chris Cormack 2013-04-29 20:06:50 UTC Comment hidden (obsolete)
Comment 8 Mason James 2013-04-30 07:17:42 UTC
Created attachment 17837 [details] [review]
Bug 10104 - make koha-disable more robust

koha-disable now:

- checks for the existence of the instance before any actions on it.
- checks if the instance is already disabled before touching anything (warns otherwise)
- only reloads apache if needed
- handles more than one instance name.
- changed the docs to acknowledge the previous item.

To test:
- Apply the patch, build your package
- Run koha-disable on
  - Non existent instance (try names that are prefix and suffix of a valid one too please)
  - Already disabled instance name.
  - Enabled instance name.

It should work as expected and warn the user on the expected wrong cases.

Regards
To+

Sponsored-by: Universidad Nacional de Córdoba
Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>
Signed-off-by: Mason James <mtj@kohaaloha.com>
Comment 9 Mason James 2013-04-30 07:19:11 UTC
Created attachment 17838 [details] [review]
Bug 10104 - Followup: fix param check

As noted by Mason on bug 10101, the check was not working for zero arguments.

Sponsored-by: Universidad Nacional de Córdoba

Signed-off-by: Mason James <mtj@kohaaloha.com>
Comment 10 Mason James 2013-04-30 07:22:24 UTC
(In reply to comment #7)
> Created attachment 17819 [details] [review] [review]
> Follow up signed off

Chris, i think you forgot to add a sign-off line to your uploaded patch
Comment 11 Mason James 2013-04-30 07:23:57 UTC
> 
> Sponsored-by: Universidad Nacional de Córdoba
> Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>
> Signed-off-by: Mason James <mtj@kohaaloha.com>

passing QA on 2 patches
Comment 12 Jared Camins-Esakov 2013-05-02 02:33:51 UTC
This patch has been pushed to master and 3.12.x.
Comment 13 Chris Cormack 2013-05-09 09:18:28 UTC
Pushed to 3.10.x will be in 3.10.6