Auto upgrade feature #220

DishantK1807 · 2016-08-11T16:02:57Z

Issue : #162

Enable disable update in the UI

System updated to latest version

Check for updates enabled

hongquan · 2016-08-16T10:07:28Z

includes/controller/user_settings_controller.php

+      $ok = false;
+      $msg .= error(_("The system is already Up-to date with the version on GitHub."), true);
+    }
+    if($ok) {


Coding style error

I'll add a blank line before the if($ok)

I guess only that's the error, or did I missed something else?

https://www.drupal.org/coding-standards#controlstruct

hongquan · 2016-08-17T08:50:52Z

update.sh

@@ -6,8 +6,8 @@
 # Description:
 #   An update bash script for engelsystem

-# Add upstream 
+# Add upstream
 git remote add upstream http://www.github.com/fossasia/engelsystem.git


Do you notice that "origin" is also "http://www.github.com/fossasia/engelsystem.git"?
No need to define a "remote" with the same URL as "origin".

If a person Forks the project and makes some minor changes to it and uses it on his/her system, then remote would be required.
In the case of forked repositories, "remote" will be different from "origin"

If he forks the project and make somes changes, he should update from his repo, not FOSSASIA repo, because who can guarantee that new change in FOSSASIA won't break his system? The code update in FOSSASIA repo is only tested against the code in FOSSASIA repo, not against code of someone else. And who guarantee that new update from FOSSASIA won't have conflict with the change he made in his repo?

I'll remove "remote" and pull from "origin"
Thank you.

hongquan · 2016-08-19T05:08:53Z

Why do you still let this happen:

when I told you to config your code editor to fix it automatically?

(You don't need to make a commit to fix it right now. Don't repeat this error in the future)

hongquan · 2016-08-20T16:34:45Z

I can make and upgrade.sql file which will contain the changes in the database scheme

Where in this PR that you run the upgrade.sql file, if that file exists?

Show me the screenshot that you have tested this feature in your localhost (with and without database change)

Can you explain this line:

echo shell_exec("sudo sh ./update.sh");

Why echo? Are you aware that in the "controller" PHP scripts, the content to return to browser is always wrapped in the functions like div, page_link_to and never returned by echo?

Why shell_exec comes with sh? Quoted from http://php.net/manual/en/function.shell-exec.php:

shell_exec — Execute command via shell...

The path ./update.sh won't work with new URL of engelsystem (changed from http://example.com/engelsystem/public/ to http://example.com/). Talk with @kamishettysreeja25 how she implemented the change and hope you will understand why that path will not work.

What if sudo ask for password? How user pass the password in?

DishantK1807 · 2016-08-20T16:38:40Z

I'll remove sudo and sh.

Why shell_exec comes with sh?

I found this on stackoverflow.com . Will find that again and post it here

hongquan · 2016-08-20T16:55:13Z

I found this on stackoverflow.com

Don't just copy and paste from stackoverflow. You have to understand what you are doing. This is not the first time I remind you to read documentation and understand the command you are using.

--- Update ---
Can you explain why you added sudo in the past, and why you will remove sudo next? If you remove sudo, will it conflict with the reason you added in the past?

sreejakshetty · 2016-08-20T17:30:30Z

includes/controller/user_settings_controller.php

@@ -187,12 +188,48 @@ function user_settings() {
        $display_message=strip_request_item('display_message');
      else
        $ok = false;
-      if($ok){
+
+      if ($ok){


Indentation error: It should be if ($ok) {

Good. Thanks!

DishantK1807 · 2016-08-20T18:49:46Z

No
removing sudo won't cause any problems. I just added it for the system which won't perform any operations without superuser permissions.
the script is running fine even without sudo.

The screenshots(without database change) are there shown above.

DishantK1807 · 2016-08-20T19:10:48Z

includes/controller/user_settings_controller.php

+      if(strcmp($current_ver, "Version: 1.0") == 0){
+        $upgrade_table = '../db/upgrade_01.sql';
+        upgrade_tables($upgrade_table);
+      }


Similarly we can create upgrade_XX.sql for every release and compare the version numbers of the of the local system.

I'll add in the documentation too, how to upgrade the database(with a sample code explaining everything) so that users who'll be working on it in future can integrate the changes in the database.

Eg. If the current version in the local system is 3.0 and on GitHub it is 5.0 then in the if statement both Upgrade_04.sql and upgrade_05.sql will run.
Will update the documentation for the same

hongquan · 2016-08-21T02:06:30Z

Now the update.sh script contains only 1 command. Let's get rid of that file.

By creating upgrade_XX.sql, you are repeating the work done by the "database migration" feature. Please look for a way to reuse that feature.

DishantK1807 · 2016-08-21T06:18:38Z

By creating upgrade_XX.sql, you are repeating the work done by the "database migration" feature. Please look for a way to reuse that feature.

@hongquan: can we implement that as a Fix/Improvement and merge this PR? I am looking for a way to reuse that feature.

Now the update.sh script contains only 1 command. Let's get rid of that file.

I suggest not to get rid of that file because we will require that while running the migrations.

hongquan · 2016-08-21T10:50:11Z

I suggest not to get rid of that file because we will require that while running the migrations

What is required is the commands in that file. Because it has only 1 command, you should pass that command to your shell_exec, instead of creating 1 more script for just that command and pass the script to shell_exec.

can we implement that as a Fix/Improvement and merge this PR

The PR is merged after it is tested to show that it is stable and not break the system. You still don't provide the screenshots to show that you have tested with update + database. It is not proved to be ready for merge yet!

--- Update ---
Don't just put an empty upgrade_01.sql and say that you already tested with database change! There has to be a real database change when you test! Include also the Apache error log here. Look into @kamishettysreeja25's PR to learn how to strip old log.

Btw, there still be whitespace error in your code.

DishantK1807 · 2016-08-23T03:20:14Z

The Apache log:
https://cryptbin.com/hUAN#369ff3ad86f47434ca748077dfc6773e

Contents of upgrade_01.sql:

 INSERT INTO `Privileges` (`id`, `name`, `desc`) VALUES (45, 'install23', 'Install Engelsystem');

Tested the system for a random database upgrade.

System Upgraded successfully

DishantK1807 · 2016-08-23T03:20:54Z

Will update the documentation.
Thank you.

hongquan · 2016-08-23T03:24:03Z

Read my #220 (comment) again:

upgrade which includes change in database schema

Change in database schema. Inserting data to database is not a change in schema.

hongquan · 2016-08-23T03:27:57Z

When you get the Apache error log, did you look into it to discover error and try to fix?

DishantK1807 · 2016-08-23T03:45:17Z

When you get the Apache error log, did you look into it to discover error and try to fix?

I will be working on fixing those errors, most of them are not because of this feature.

Change in database schema. Inserting data to database is not a change in schema.

I'll test for change in schema as well

DishantK1807 · 2016-08-23T03:59:53Z

The Apache log:
http://cryptb.in/NEowIW#ad4eed3bf192e327e1c31fed2df8e234

content of upgrade_01.sql

ALTER TABLE `Room`
  ADD `e_id` int(11) NOT NULL;

System Upgraded successfully.

hongquan · 2016-08-23T04:12:43Z

I reminded you to look into Apache error log, but you still ignore this:

[Tue Aug 23 09:26:00.998777 2016] [:error] [pid 3074] [client 127.0.0.1:41481] PHP Warning: file_get_contents(raw.githubusercontent.com/fossasia/engelsystem/master/Version.txt): failed to open stream: HTTP request failed! HTTP/1.1 404 Not Found\r\n in /var/www/html/engelsystem/public/index.php on line 34
[Tue Aug 23 09:26:00.998832 2016] [:error] [pid 3074] [client 127.0.0.1:41481] PHP Warning: file_get_contents( ../Version.txt): failed to open stream: No such file or directory in /var/www/html/engelsystem/public/index.php on line 35

Look into kamishettysreeja25 PR about "Wordpress-like installation" to learn how to prevent the other errors. And also you can merge "development" branch to your branch to update with the fix she has done.

hongquan · 2016-08-23T04:17:40Z

      if(strcmp($current_ver, "Version: 1.0") == 0){
        $upgrade_table = '../db/upgrade_01.sql';
        upgrade_tables($upgrade_table);
      }

If Version.txt contains higher number, like Version: 2.0, the database migration won't happen, right? And then the system will break?

DishantK1807 · 2016-08-23T04:19:13Z

If Version.txt contains higher number, like Version: 2.0, the database migration won't happen, right? And then the system will break?

I explained that in the Line comment, https://github.com/fossasia/engelsystem/pull/220/files#r75583290
That we would need to update code the similar way.

I'll add that to the documentation/wiki

DishantK1807 · 2016-08-23T04:33:39Z

Apache log:
http://cryptb.in/C7hr#10db833189ee8869546c86359431e522

Fixed the mentioned error. the error was due to the wrong link in index.php
Changed the link in local repo and tested.

I'll look into Kamishettysreeja PR for solving the other errors

hongquan · 2016-08-23T04:35:49Z

What? You ask user to modify source code? He/she has to find which version his/her website is, and which version the new update is, and has to implement the whole if code block? Then how come this feature is called "auto upgrade"? This feature has been existed in many PHP web system like Wordpress, Drupal, Piwik but I have never seen one asking user to modify source code to upgrade.

Needless to say that user doesn't know when to change, because:

If user modifies before the upgrade process happens, the "git pull" command will stop if the new update contains update for the file user has modified. And just finding out the version number is already headache for user.
If user modifies after "git pull", no chance. The process won't pause for user to intervene.

DishantK1807 · 2016-08-23T04:45:52Z

What? You ask user to modify source code? He/she has to find which version his/her website is, and which version the new update is, and has to implement the whole if code block? Then how come this feature is called "auto upgrade"? This feature has been existed in many PHP web system like Wordpress, Drupal, Piwik but I have never seen one asking user to modify source code to upgrade.
Needless to say that user doesn't know when to change, because:
If user modify before the upgrade process happen, the "git pull" command will stop if the new update contains update for the file user has modified. Just find out the version number is already headache for user.
If user modify after "git pull", no chance. The process won't pause for user to intervene.

No. not the user. User won't work on the code.
The person contributing the code.
Will add this to the wiki as a condition when the PR by the contributor is merged.

hongquan · 2016-08-23T05:01:45Z

The person contributing the code.
If the current version in the local system is 3.0 and on GitHub it is 5.0 then in the if statement both Upgrade_04.sql and upgrade_05.sql will run.
Will update the documentation for the same

Then how the developer know which version the local system is? Assume you are releasing v5, but Mario has a website with v4, and sister Phuc has a website with v1. How the developer implement that if block to satisfy both?

--- Update ---
Second time to remind you about coding style error.

DishantK1807 added 6 commits August 7, 2016 22:31

Created Update script

3597801

Created version file

867c6d2

Update update.sql

92204d7

Added update feature

dd482c8

Updated setting_model for auto_update

c6af966

Implemented automatic check for updates

1fbc9a5

DishantK1807 added enhancement feature design database deployment labels Aug 11, 2016

DishantK1807 added this to the GSOC 2016 Milestone 13 milestone Aug 11, 2016

DishantK1807 self-assigned this Aug 11, 2016

This was referenced Aug 11, 2016

Added enable/disable update feature #219

Open

Implement an upgrade process for Engelsystem similar to Wordpress #162

Open

DishantK1807 added 2 commits August 11, 2016 21:37

changed the messages displayed

011157a

Updated documentation for AutoUpdates

5170996

mariobehling modified the milestones: GSOC 2016 Milestone 13, GSOC 2016 Milestone 14 (Final Evaluation) Aug 15, 2016

hongquan reviewed Aug 16, 2016
View reviewed changes

DishantK1807 added 2 commits August 17, 2016 08:53

Fix: Coding Style errors

ff8b0a3

Changed links

25dbca9

hongquan reviewed Aug 17, 2016
View reviewed changes

change upstream to origin

ce8c668

DishantK1807 force-pushed the auto-update branch from 04803c7 to ce8c668 Compare August 20, 2016 03:43

sreejakshetty reviewed Aug 20, 2016
View reviewed changes

DishantK1807 added 2 commits August 21, 2016 00:35

changed links

d65cffa

Fix: upgrade procedure

060232b

DishantK1807 reviewed Aug 20, 2016
View reviewed changes

Fix: Indentataion errors

3fb9cd1

DishantK1807 added 2 commits August 23, 2016 08:13

resolving conflicts

1362c99

Removed the Shell file

657ff02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Auto upgrade feature #220

Auto upgrade feature #220

DishantK1807 commented Aug 11, 2016

hongquan Aug 16, 2016

DishantK1807 Aug 16, 2016

DishantK1807 Aug 16, 2016

hongquan Aug 17, 2016

hongquan Aug 17, 2016

DishantK1807 Aug 17, 2016

hongquan Aug 17, 2016

DishantK1807 Aug 17, 2016

hongquan commented Aug 19, 2016

hongquan commented Aug 20, 2016

DishantK1807 commented Aug 20, 2016 •

edited

Loading

hongquan commented Aug 20, 2016 •

edited

Loading

sreejakshetty Aug 20, 2016

hongquan Aug 21, 2016

DishantK1807 commented Aug 20, 2016 •

edited

Loading

DishantK1807 Aug 20, 2016 •

edited

Loading

hongquan commented Aug 21, 2016

DishantK1807 commented Aug 21, 2016

hongquan commented Aug 21, 2016 •

edited

Loading

DishantK1807 commented Aug 23, 2016

DishantK1807 commented Aug 23, 2016

hongquan commented Aug 23, 2016

hongquan commented Aug 23, 2016

DishantK1807 commented Aug 23, 2016

DishantK1807 commented Aug 23, 2016

hongquan commented Aug 23, 2016

hongquan commented Aug 23, 2016

DishantK1807 commented Aug 23, 2016

DishantK1807 commented Aug 23, 2016 •

edited

Loading

hongquan commented Aug 23, 2016 •

edited

Loading

DishantK1807 commented Aug 23, 2016 •

edited

Loading

hongquan commented Aug 23, 2016 •

edited

Loading

Auto upgrade feature #220

Are you sure you want to change the base?

Auto upgrade feature #220

Conversation

DishantK1807 commented Aug 11, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hongquan commented Aug 19, 2016

hongquan commented Aug 20, 2016

DishantK1807 commented Aug 20, 2016 • edited Loading

hongquan commented Aug 20, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DishantK1807 commented Aug 20, 2016 • edited Loading

DishantK1807 Aug 20, 2016 • edited Loading

Choose a reason for hiding this comment

hongquan commented Aug 21, 2016

DishantK1807 commented Aug 21, 2016

hongquan commented Aug 21, 2016 • edited Loading

DishantK1807 commented Aug 23, 2016

DishantK1807 commented Aug 23, 2016

hongquan commented Aug 23, 2016

hongquan commented Aug 23, 2016

DishantK1807 commented Aug 23, 2016

DishantK1807 commented Aug 23, 2016

hongquan commented Aug 23, 2016

hongquan commented Aug 23, 2016

DishantK1807 commented Aug 23, 2016

DishantK1807 commented Aug 23, 2016 • edited Loading

hongquan commented Aug 23, 2016 • edited Loading

DishantK1807 commented Aug 23, 2016 • edited Loading

hongquan commented Aug 23, 2016 • edited Loading

DishantK1807 commented Aug 20, 2016 •

edited

Loading

hongquan commented Aug 20, 2016 •

edited

Loading

DishantK1807 commented Aug 20, 2016 •

edited

Loading

DishantK1807 Aug 20, 2016 •

edited

Loading

hongquan commented Aug 21, 2016 •

edited

Loading

DishantK1807 commented Aug 23, 2016 •

edited

Loading

hongquan commented Aug 23, 2016 •

edited

Loading

DishantK1807 commented Aug 23, 2016 •

edited

Loading

hongquan commented Aug 23, 2016 •

edited

Loading