Hi all, Patch 7167 has been signed off. I have pasted my QA comment here below to get some feedback from you. Note that this patch will impact your future updates ! My comments are not strictly technical, but also on a functional level. Due to the nature of this patch, I think that I should. Before Paul or Jonathan moves on, you may have some comments. Do you agree with me, or why not? Etc. Thanks a lot.. Marcel QA comment: Larger patch with greater impact. Good work! Code generally looks good. Nice idea. Will make testing patches with dbrevs easier. Because of the impact of this patch, I think that we still need some adjustments before it passes qa. The points below may look like a lot of work, but will not need that much code changes. I believe that it can be done within some hours. (Please note that I would really LIKE to see this feature be pushed soon!) I change status to In Discussion and will ask for input from the dev list. Before you change the patch, it would be good to have some additional community consensus about it (when given). My main (functional) point is actually: make a distinction between numbered dbrevs (from master) and unnumbered dbrevs (from test patches or custom development). An upgrade should always run the numbered dbrevs in linear order. The unnumbered ones can be executed in any order from the new update form if you are in dev mode. This makes a wiki with picking dbrev numbers unneeded. The RM still takes care of the numbering (rename a file). A developer just adds a file without dbrev number in a new patch, perhaps referrring to a bug report. There will be no gaps in the numbering scheme. About lists the correct official Koha version and the applied 'custom' dbrevs. YAML: As I understand, the idea was initially to have dbrevs in a yaml file. In the current implementation, only the version dir is in that file. Additionally, you do not use the dirload, only the fileload. For simplicity, I recommend to remove the YAML file and associated code. The versions directory can be hardcoded in my opinion within the install/data.. region. I would suggest to rename it to dbrev or something similar, and have numbered version subdirectories within it. Example: This will eventually lead to dirs and files like dbrevs/3.09/3.09.00.001.sql or dbrevs/3.10/3.10.02.003.pl, etc. Development mode: Currently, you rely on $ENV{DEBUG} from koha-httpd file. I would turn this into a systempref. It is easier for developers to toggle it (compared to restarting httpd service). Skeleton: I see it in a regex as well as some code inserting into foo. Assume that it was used in testing. Please remove it. In actual use, you will find enough examples to find your way. Or just put some comments into a readme file. Mainpage: Some code was removed from Auth.pm. The version check is now in mainpage.pl itself. I would rather have that check in an appropriate module. It was in Auth.pm. You could leave it there? I think we should let the version check stay within the scope of the checkauth call included in the get_user_and_template call. (See also comment on Auth.pm below). Authorization (Auth.pm): I agree that if a user already logged in, it is not needed to check the version every time. The ("new") initial check in checkauth if the db contains a version, is fine. The former moment of the "complete" version check was too early. I think we should leave it in checkauth, but do it at a later moment: If we start a new session and we verified the password, then we should do the extended version check (database versus kohaversion.pl). That is close to line 748 in the adjusted Auth.pm (depending on the value of $return). This will prevent Koha from checking it every time, but only for a new session. If the version check fails at that moment, we should redirect to installer step 3 (updatestructure). Note that there is some problem in current code, that forces me to login twice when there is an update available. (It redirects to admin/updatedatabase, but I must relogin again.) Please correct. Probably you should only pass the $cookie in your redirect call too. Like: print $query->redirect(-uri=>"/cgi-bin/koha/admin/updatedatabase.pl",-cookie=>$cookie); Upgrading: When coming from an older version (before 3.9.0.x), you should run the old updatedatabase and after that you should run the new dbrevs in the dbrev directories. This is currently done in two passes. First it runs the old update. You think that it is ready. When you login, you are prompted to the new update form. It should not be too difficult to merge this into one pass (less confusing). Please adjust install.pl for that: You should check if it is still needed to call the old update before running the new one (for numbered dbrevs only). Update form: Since numbered dbrevs are run via installer, only allow executing unnumbered dbrevs here (in dev mode). In normal mode, you can only browse history here. (I would suggest to not even show the unnumbered dbrevs if you are in normal mode.) File structure: As mentioned above, make directories for a Koha release. Getting all updates means fetching the updates from a few directories. This makes the feature more future-proof. Stable version: The md5 test will be of good use here! If we backport this to 3.8.X, we could already check what updates we already have run from 3.9 folder with the md5 checksum. When upgrading from 3.8 to 3.10, some dbrevs are new, others will be incorporated already. So this remark only serves the purpose of discussing "Should we also get this into stable already (within reasonable time)?". admin/updatedatabase.pl: Line 33 adds a fixme to new code: Add a new flag? Commit message: please make it up-to-date. E.g. section on installdir.
Marcel, et. al., Patch 7167 has been signed off. I have pasted my QA comment here below to
get some feedback from you. Note that this patch will impact your future updates !
My comments are not strictly technical, but also on a functional level. Due to the nature of this patch, I think that I should.
Before Paul or Jonathan moves on, you may have some comments. Do you agree with me, or why not? Etc.
Thanks a lot..
Marcel
QA comment: Larger patch with greater impact. Good work! Code generally looks good. Nice idea. Will make testing patches with dbrevs easier. Because of the impact of this patch, I think that we still need some adjustments before it passes qa. The points below may look like a lot of work, but will not need that much code changes. I believe that it can be done within some hours. (Please note that I would really LIKE to see this feature be pushed soon!)
I change status to In Discussion and will ask for input from the dev list. Before you change the patch, it would be good to have some additional community consensus about it (when given).
My main (functional) point is actually: make a distinction between numbered dbrevs (from master) and unnumbered dbrevs (from test patches or custom development). An upgrade should always run the numbered dbrevs in linear order. The unnumbered ones can be executed in any order from the new update form if you are in dev mode. This makes a wiki with picking dbrev numbers unneeded. The RM still takes care of the numbering (rename a file). A developer just adds a file without dbrev number in a new patch, perhaps referrring to a bug report. There will be no gaps in the numbering scheme. About lists the correct official Koha version and the applied 'custom' dbrevs.
I think this is a fantastic idea. I think this would greatly simplify the development and testing process.
YAML: As I understand, the idea was initially to have dbrevs in a yaml file. In the current implementation, only the version dir is in that file. Additionally, you do not use the dirload, only the fileload. For simplicity, I recommend to remove the YAML file and associated code. The versions directory can be hardcoded in my opinion within the install/data.. region. I would suggest to rename it to dbrev or something similar, and have numbered version subdirectories within it. Example: This will eventually lead to dirs and files like dbrevs/3.09/3.09.00.001.sql or dbrevs/3.10/3.10.02.003.pl, etc.
Development mode: Currently, you rely on $ENV{DEBUG} from koha-httpd file. I would turn this into a systempref. It is easier for developers to toggle it (compared to restarting httpd service).
It's easier, but also a lot more dangerous in my opinion. The consensus on the caching configuration was that it was too dangerous to allow access to it in sysprefs. Based on that logic, I think using an environment variable would be the way to go.
Skeleton: I see it in a regex as well as some code inserting into foo. Assume that it was used in testing. Please remove it. In actual use, you will find enough examples to find your way. Or just put some comments into a readme file.
Mainpage: Some code was removed from Auth.pm. The version check is now in mainpage.pl itself. I would rather have that check in an appropriate module. It was in Auth.pm. You could leave it there? I think we should let the version check stay within the scope of the checkauth call included in the get_user_and_template call. (See also comment on Auth.pm below).
Authorization (Auth.pm): I agree that if a user already logged in, it is not needed to check the version every time. The ("new") initial check in checkauth if the db contains a version, is fine. The former moment of the "complete" version check was too early. I think we should leave it in checkauth, but do it at a later moment: If we start a new session and we verified the password, then we should do the extended version check (database versus kohaversion.pl). That is close to line 748 in the adjusted Auth.pm (depending on the value of $return). This will prevent Koha from checking it every time, but only for a new session. If the version check fails at that moment, we should redirect to installer step 3 (updatestructure).
Note that there is some problem in current code, that forces me to login twice when there is an update available. (It redirects to admin/updatedatabase, but I must relogin again.) Please correct. Probably you should only pass the $cookie in your redirect call too. Like: print $query->redirect(-uri=>"/cgi-bin/koha/admin/updatedatabase.pl ",-cookie=>$cookie);
Upgrading: When coming from an older version (before 3.9.0.x), you should run the old updatedatabase and after that you should run the new dbrevs in the dbrev directories. This is currently done in two passes. First it runs the old update. You think that it is ready. When you login, you are prompted to the new update form. It should not be too difficult to merge this into one pass (less confusing). Please adjust install.pl for that: You should check if it is still needed to call the old update before running the new one (for numbered dbrevs only).
Update form: Since numbered dbrevs are run via installer, only allow executing unnumbered dbrevs here (in dev mode). In normal mode, you can only browse history here. (I would suggest to not even show the unnumbered dbrevs if you are in normal mode.)
I disagree here, too. I think unnumbered dbrevs should be shown in normal mode, so that if a developer switches to normal mode, they're able to spot "oh, oops, I forgot to reset my database."
File structure: As mentioned above, make directories for a Koha release. Getting all updates means fetching the updates from a few directories. This makes the feature more future-proof.
Stable version: The md5 test will be of good use here! If we backport this to 3.8.X, we could already check what updates we already have run from 3.9 folder with the md5 checksum. When upgrading from 3.8 to 3.10, some dbrevs are new, others will be incorporated already. So this remark only serves the purpose of discussing "Should we also get this into stable already (within reasonable time)?".
admin/updatedatabase.pl: Line 33 adds a fixme to new code: Add a new flag?
Commit message: please make it up-to-date. E.g. section on installdir.
Regards, Jared -- Jared Camins-Esakov Bibliographer, C & P Bibliography Services, LLC (phone) +1 (917) 727-3445 (e-mail) jcamins@cpbibliography.com (web) http://www.cpbibliography.com/
Op 13-06-12 15:39, Jared Camins-Esakov schreef:
Development mode: Currently, you rely on $ENV{DEBUG} from koha-httpd file. I would turn this into a systempref. It is easier for developers to toggle it (compared to restarting httpd service).
It's easier, but also a lot more dangerous in my opinion. The consensus on the caching configuration was that it was too dangerous to allow access to it in sysprefs. Based on that logic, I think using an environment variable would be the way to go.
My 2p right now is to agree with this, as someone who provides hosted Koha to people who don't necessarily know the ins-and-outs of it all, I can see having sysadminny-type options in the sysprefs in general causing issues. I'd advocate some standard for these kinds of things in the long term[0], but for the time being, I think an environment variable could work. Robin. [0] other examples I can think of that are impending: the filesystem location of custom XSLT and files that are attached to bib records but really live on the filesystem. These should be defined in a location that only a sysadmin can change them.
Just bringing this back to original intent: it's hard for developers and testers to work with patches that have DB revs, so we want to fix that. Basically, that means being able to easily tell what changes have been made to the DB beyond the natural Koha progression. The problem with the code we have, I think, is that we're trying to make this something that can be managed in the staff client. Our target audience here isn't Koha users, it's testers and developers. Whatever we're doing to address our needs as code maintainers must not effect the people actually using the software. I've floated this before, so I will now again: I think we need to move our database updates to individual Perl files, with a set API: DESCRIBE, CHECK, DO and UNDO. If every atomic update contains the necessary logic to handle these four functions, we'll be able to: 1. see what the change is BEFORE applying it 2. see if the change is necessary BEFORE applying it 3. revert changes after testing them updatedatabase, then, becomes the mechanism by which the RM assigns these Perl files a linear sequence and distinct DB rev number. Any change that's running "on top" of the core Koha code won't need a temporary/fake number assigned to it to get added. Maintaining it will be the job of the sysadmin who decided to run non-core code to begin with, but that's nothing new. It's very rare that sequence is important for DB revs, as we have few that change the table structure, and those that do often do not interact in a short span of time. If you find a Koha DB rev doesn't apply to your customized DB structure, it's time for a rebase! I really don't think this is a frequent enough occurrence to warrant too much consideration. In short, I think we should hold off on our eagerness to push this change through, and wait until we've come up with a different solution. We're only putting the users of Koha at risk for serious DB-related bugs, with no tangible gain in it for them. Cheers, -Ian On Fri, Jun 15, 2012 at 7:10 AM, Robin Sheat <robin@catalyst.net.nz> wrote:
Op 13-06-12 15:39, Jared Camins-Esakov schreef:
Development mode: Currently, you rely on $ENV{DEBUG} from koha-httpd file. I would turn this into a systempref. It is easier for developers to toggle it (compared to restarting httpd service).
It's easier, but also a lot more dangerous in my opinion. The consensus on the caching configuration was that it was too dangerous to allow access to it in sysprefs. Based on that logic, I think using an environment variable would be the way to go.
My 2p right now is to agree with this, as someone who provides hosted Koha to people who don't necessarily know the ins-and-outs of it all, I can see having sysadminny-type options in the sysprefs in general causing issues.
I'd advocate some standard for these kinds of things in the long term[0], but for the time being, I think an environment variable could work.
Robin.
[0] other examples I can think of that are impending: the filesystem location of custom XSLT and files that are attached to bib records but really live on the filesystem. These should be defined in a location that only a sysadmin can change them. _______________________________________________ Koha-devel mailing list Koha-devel@lists.koha-community.org http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-devel website : http://www.koha-community.org/ git : http://git.koha-community.org/ bugs : http://bugs.koha-community.org/
On 2012-06-16, at 6:51 AM, Ian Walls wrote:
Just bringing this back to original intent: it's hard for developers and testers to work with patches that have DB revs, so we want to fix that. Basically, that means being able to easily tell what changes have been made to the DB beyond the natural Koha progression.
The problem with the code we have, I think, is that we're trying to make this something that can be managed in the staff client. Our target audience here isn't Koha users, it's testers and developers. Whatever we're doing to address our needs as code maintainers must not effect the people actually using the software.
I've floated this before, so I will now again: I think we need to move our database updates to individual Perl files, with a set API: DESCRIBE, CHECK, DO and UNDO. If every atomic update contains the necessary logic to handle these four functions, we'll be able to:
1. see what the change is BEFORE applying it 2. see if the change is necessary BEFORE applying it 3. revert changes after testing them
updatedatabase, then, becomes the mechanism by which the RM assigns these Perl files a linear sequence and distinct DB rev number. Any change that's running "on top" of the core Koha code won't need a temporary/fake number assigned to it to get added. Maintaining it will be the job of the sysadmin who decided to run non-core code to begin with, but that's nothing new.
It's very rare that sequence is important for DB revs, as we have few that change the table structure, and those that do often do not interact in a short span of time. If you find a Koha DB rev doesn't apply to your customized DB structure, it's time for a rebase! I really don't think this is a frequent enough occurrence to warrant too much consideration.
In short, I think we should hold off on our eagerness to push this change through, and wait until we've come up with a different solution. We're only putting the users of Koha at risk for serious DB-related bugs, with no tangible gain in it for them.
firstly, i love Ian's solution, i think its the ultimate solution for db-upgrades in Koha. (and i think most devs would agree?) i think we should accept Paul's patch, and plan to add Ian's 'DESCRIBE, CHECK, DO, UNDO' functionally to it, in a later patch Paul's solution is missing the ability to CHECK (and UNDO?) db-patches if we add those features to Paul's patch, we basically have Ian's solution, am i right here? surely this is the best way towards a solution here? rather than abandoning Paul's patch for a better patch, that does not yet exist
Thanks your feedback until now! I tend to agree more with Mason here. If Paul adjusts his patch to meet QA, let's move forward with that patch. Adding the REVERT logic would make it even better later on. To get the discussion further, I think we still need two answers: 1) Should we add the distinction between numbered approved dbrevs and unnumbered ones in testing stage that I suggested in my QA comments? 2) As Jared suggested, should we leave enabling the development mode of this feature in an environment variable (DEBUG, set in Apache conf)? Marcel ________________________________________ Van: koha-devel-bounces@lists.koha-community.org [koha-devel-bounces@lists.koha-community.org] namens Mason James [mtj@kohaaloha.com] Verzonden: zaterdag 16 juni 2012 5:37 To: Ian Walls Cc: koha-devel@lists.koha-community.org Onderwerp: Re: [Koha-devel] Bug 7167 New updatedatabase On 2012-06-16, at 6:51 AM, Ian Walls wrote:
Just bringing this back to original intent: it's hard for developers and testers to work with patches that have DB revs, so we want to fix that. Basically, that means being able to easily tell what changes have been made to the DB beyond the natural Koha progression.
The problem with the code we have, I think, is that we're trying to make this something that can be managed in the staff client. Our target audience here isn't Koha users, it's testers and developers. Whatever we're doing to address our needs as code maintainers must not effect the people actually using the software.
I've floated this before, so I will now again: I think we need to move our database updates to individual Perl files, with a set API: DESCRIBE, CHECK, DO and UNDO. If every atomic update contains the necessary logic to handle these four functions, we'll be able to:
1. see what the change is BEFORE applying it 2. see if the change is necessary BEFORE applying it 3. revert changes after testing them
updatedatabase, then, becomes the mechanism by which the RM assigns these Perl files a linear sequence and distinct DB rev number. Any change that's running "on top" of the core Koha code won't need a temporary/fake number assigned to it to get added. Maintaining it will be the job of the sysadmin who decided to run non-core code to begin with, but that's nothing new.
It's very rare that sequence is important for DB revs, as we have few that change the table structure, and those that do often do not interact in a short span of time. If you find a Koha DB rev doesn't apply to your customized DB structure, it's time for a rebase! I really don't think this is a frequent enough occurrence to warrant too much consideration.
In short, I think we should hold off on our eagerness to push this change through, and wait until we've come up with a different solution. We're only putting the users of Koha at risk for serious DB-related bugs, with no tangible gain in it for them.
firstly, i love Ian's solution, i think its the ultimate solution for db-upgrades in Koha. (and i think most devs would agree?) i think we should accept Paul's patch, and plan to add Ian's 'DESCRIBE, CHECK, DO, UNDO' functionally to it, in a later patch Paul's solution is missing the ability to CHECK (and UNDO?) db-patches if we add those features to Paul's patch, we basically have Ian's solution, am i right here? surely this is the best way towards a solution here? rather than abandoning Paul's patch for a better patch, that does not yet exist
On Sat, Jun 16, 2012 at 2:40 AM, Marcel de Rooy <M.de.Rooy@rijksmuseum.nl>wrote:
Thanks your feedback until now! I tend to agree more with Mason here. If Paul adjusts his patch to meet QA, let's move forward with that patch. Adding the REVERT logic would make it even better later on.
I agree w/Mason also. We can build on Paul's work going forward.
To get the discussion further, I think we still need two answers: 1) Should we add the distinction between numbered approved dbrevs and unnumbered ones in testing stage that I suggested in my QA comments?
+1 I think the RM should have the job of choosing DB numbers in any case. That removes a bunch of administrative type headaches.
2) As Jared suggested, should we leave enabling the development mode of this feature in an environment variable (DEBUG, set in Apache conf)?
+1 Here again, this is a sysadmin type function. Moving it to sysprefs is asking for trouble from curious users imho. Kind Regards, Chris
participants (6)
-
Chris Nighswonger -
Ian Walls -
Jared Camins-Esakov -
Marcel de Rooy -
Mason James -
Robin Sheat