New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[stable9] Add repair step for updater issues #24134

Merged
merged 1 commit into from Apr 21, 2016

Conversation

Projects
None yet
7 participants
@LukasReschke
Member

LukasReschke commented Apr 20, 2016

The updater as shipped with ownCloud =< 9.0.1 has several bugs leading to a not properly executed update. For example the third-party changes are not copied.

This pull request:

  1. Ships the third-party files changed since ownCloud 9.0.1 in the resources folder. On update the files are replaced. (owncloud/updater#316)
  2. Adds updater/* and _oc_upgrade/* as an exemption to the code integrity checker since the updater is updating in the wrong order. (owncloud/updater#318)

Most of this code are actually the missed files. This is what is included in resources/.

@DeepDiver1975 @karlitschek @VicDeo Please review.
@karlitschek Please approve.

I'd recommend to get this in as soon as possible so we can have some proper testing on this including also the daily channel.

Fixes #23997
Fixes #23857

Add repair step for updater issues
The updater as shipped with ownCloud =< 9.0.1 has several bugs leading to a not properly executed update. For example the third-party changes are not copied.

This pull request:

1. Ships the third-party files changed since ownCloud 9.0.1 in the resources folder. On update the files are replaced. (owncloud/updater#316)
2. Adds updater/* and _oc_upgrade/* as an exemption to the code integrity checker since the updater is updating in the wrong order. (owncloud/updater#318)

@LukasReschke LukasReschke added this to the 9.0.2-current-maintenance milestone Apr 20, 2016

@mention-bot

This comment has been minimized.

Show comment
Hide comment
@mention-bot

mention-bot Apr 20, 2016

By analyzing the blame information on this pull request, we identified @PVince81, @DeepDiver1975 and @nickvergessen to be potential reviewers

mention-bot commented Apr 20, 2016

By analyzing the blame information on this pull request, we identified @PVince81, @DeepDiver1975 and @nickvergessen to be potential reviewers

@LukasReschke

This comment has been minimized.

Show comment
Hide comment
Member

LukasReschke commented Apr 20, 2016

@karlitschek

This comment has been minimized.

Show comment
Hide comment
@karlitschek

karlitschek Apr 20, 2016

Member

not tested but looks ok. As discussed. 👍 We need some full testing for that somehow.

Member

karlitschek commented Apr 20, 2016

not tested but looks ok. As discussed. 👍 We need some full testing for that somehow.

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Apr 21, 2016

Member

@LukasReschke I guess we don't need a master PR and this PR is to cleanup the mess that was done by the updater between 9.0.1 and 9.0.2 ?

Member

PVince81 commented Apr 21, 2016

@LukasReschke I guess we don't need a master PR and this PR is to cleanup the mess that was done by the updater between 9.0.1 and 9.0.2 ?

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Apr 21, 2016

Member

Are there any steps to reproduce for QA to test this ?

@VicDeo would be good if you could help to quickly review and test this so we can get it merged for tomorrow's daily.

CC @owncloud/qa

Member

PVince81 commented Apr 21, 2016

Are there any steps to reproduce for QA to test this ?

@VicDeo would be good if you could help to quickly review and test this so we can get it merged for tomorrow's daily.

CC @owncloud/qa

@VicDeo

This comment has been minimized.

Show comment
Hide comment
@VicDeo

VicDeo Apr 21, 2016

Member

According to @DeepDiver1975 updater should be able to update itself separately from core update (not implemented yet)
so updater/* should not be checked with core but separately (and against a separate signatures.json).

Member

VicDeo commented Apr 21, 2016

According to @DeepDiver1975 updater should be able to update itself separately from core update (not implemented yet)
so updater/* should not be checked with core but separately (and against a separate signatures.json).

@LukasReschke

This comment has been minimized.

Show comment
Hide comment
@LukasReschke

LukasReschke Apr 21, 2016

Member

According to @DeepDiver1975 updater should be able to update itself separately from core update (not implemented yet)
so updater/* should not be checked with core.

The PR adds this. HOWEVER, I'm not willed to add this to master as well until this has actually been implemented in the updater as well.

Member

LukasReschke commented Apr 21, 2016

According to @DeepDiver1975 updater should be able to update itself separately from core update (not implemented yet)
so updater/* should not be checked with core.

The PR adds this. HOWEVER, I'm not willed to add this to master as well until this has actually been implemented in the updater as well.

@LukasReschke

This comment has been minimized.

Show comment
Hide comment
@LukasReschke

LukasReschke Apr 21, 2016

Member

@LukasReschke I guess we don't need a master PR and this PR is to cleanup the mess that was done by the updater between 9.0.1 and 9.0.2 ?

Correct, and 9.0.0 to 9.0.2.

Are there any steps to reproduce for QA to test this ?

That's kinda hard to test reliably, the easiest way would be to get this into the stable9 branch and test to update from 9.0.0 to 9.0-daily once this has been merged. Another way would be to modify the code to download a build with this changes included.

So I'd say there is no real way to test this except waiting for being this in daily. I mean I surely can post the ways that I tested this locally here but it will involve modifying 5 files in ownCloud and is not really a representative real life test 😢

So probably not worth it to 100% retest what I already tested. I seriously see only the daily branch as valid testing approach.

Member

LukasReschke commented Apr 21, 2016

@LukasReschke I guess we don't need a master PR and this PR is to cleanup the mess that was done by the updater between 9.0.1 and 9.0.2 ?

Correct, and 9.0.0 to 9.0.2.

Are there any steps to reproduce for QA to test this ?

That's kinda hard to test reliably, the easiest way would be to get this into the stable9 branch and test to update from 9.0.0 to 9.0-daily once this has been merged. Another way would be to modify the code to download a build with this changes included.

So I'd say there is no real way to test this except waiting for being this in daily. I mean I surely can post the ways that I tested this locally here but it will involve modifying 5 files in ownCloud and is not really a representative real life test 😢

So probably not worth it to 100% retest what I already tested. I seriously see only the daily branch as valid testing approach.

@VicDeo

This comment has been minimized.

Show comment
Hide comment
@VicDeo

VicDeo Apr 21, 2016

Member

@LukasReschke Looks good, I was about to start testing in a hard way.
👍

Member

VicDeo commented Apr 21, 2016

@LukasReschke Looks good, I was about to start testing in a hard way.
👍

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Apr 21, 2016

Member

@LukasReschke @VicDeo Ok then, once this is on daily, what are the steps to reproduce the issue and run the fix ?

Member

PVince81 commented Apr 21, 2016

@LukasReschke @VicDeo Ok then, once this is on daily, what are the steps to reproduce the issue and run the fix ?

@nickvergessen

This comment has been minimized.

Show comment
Hide comment
@nickvergessen

nickvergessen Apr 21, 2016

Contributor

@PVince81
Install 9.0.0
Update to 9.0.1
Run code checker =>

Install 9.0.0
Update to daily
Run code checker

Contributor

nickvergessen commented Apr 21, 2016

@PVince81
Install 9.0.0
Update to 9.0.1
Run code checker =>

Install 9.0.0
Update to daily
Run code checker

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Apr 21, 2016

Member

@nickvergessen thanks, did you get that @owncloud/qa ? 😄

Merging then

Member

PVince81 commented Apr 21, 2016

@nickvergessen thanks, did you get that @owncloud/qa ? 😄

Merging then

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Apr 21, 2016

Member

Button grayed out 😦

@DeepDiver1975 can you merge this ?

Member

PVince81 commented Apr 21, 2016

Button grayed out 😦

@DeepDiver1975 can you merge this ?

@DeepDiver1975 DeepDiver1975 merged commit 80993b2 into stable9 Apr 21, 2016

22 checks passed

cla-bot-core Build #3309 succeeded in 47 sec
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
core-ci-linux-jsunit/database=sqlite,label=SLAVE Build #60964 succeeded in 3 min 50 sec
Details
core-ci-linux-swift-primary-storage/database=mysql,label=SLAVE Build #55098 succeeded in 19 min
Details
core-ci-linux/database=mysql,label=SLAVE Build #29593 succeeded in 26 min
Details
core-ci-linux/database=oci,label=SLAVE Build #29593 succeeded in 45 min
Details
core-ci-linux/database=pgsql,label=SLAVE Build #29593 succeeded in 26 min
Details
core-ci-linux/database=sqlite,label=SLAVE Build #29593 succeeded in 17 min
Details
ocs-api-integration-tests-ci Build #9837 succeeded in 11 min
Details
server-master-linux-externals-ci/database=sqlite,external=smb-silvershell,label=SLAVE Build #9519 succeeded in 6 min 54 sec
Details
server-master-linux-externals-ci/database=sqlite,external=swift-ceph,label=SLAVE Build #9519 succeeded in 7 min 42 sec
Details
server-master-linux-externals-ci/database=sqlite,external=webdav-ownCloud,label=SLAVE Build #9519 succeeded in 8 min 49 sec
Details
server-master-linux-externals-smb-windows-ext-ci/database=sqlite,external=smb-windows,label=master Build #11860 succeeded in 2 min 28 sec
Details
server-master-linux-php5.4-ci/database=sqlite,label=SLAVE Build #2467 succeeded in 7 min 53 sec
Details
server-master-linux-php7-ci/database=sqlite,label=SLAVE Build #37796 succeeded in 12 min
Details
smashbox-on-docker-ci/DOCKER_IMAGE=ubuntu_oc_lamp-git,TEST_NAME=litmus,mirallBranch=v2.0.2,slave=SMASH Build #13835 succeeded in 5 min 33 sec
Details
smashbox-on-docker-ci/DOCKER_IMAGE=ubuntu_oc_lamp-git,TEST_NAME=test_basicSync@0,mirallBranch=v2.0.2,slave=SMASH Build #13835 succeeded in 20 min
Details
smashbox-on-docker-ci/DOCKER_IMAGE=ubuntu_oc_lamp-git,TEST_NAME=test_basicSync@1,mirallBranch=v2.0.2,slave=SMASH Build #13835 succeeded in 32 min
Details
smashbox-on-docker-ci/DOCKER_IMAGE=ubuntu_oc_lamp-git,TEST_NAME=test_shareLink,mirallBranch=v2.0.2,slave=SMASH Build #13835 succeeded in 30 min
Details
smashbox-on-docker-ci/DOCKER_IMAGE=ubuntu_oc_lamp-git,TEST_NAME=test_sharePermissions,mirallBranch=v2.0.2,slave=SMASH Build #13835 succeeded in 18 min
Details
smashbox-on-docker-ci/DOCKER_IMAGE=ubuntu_oc_lamp-git,TEST_NAME=test_sharePropagationGroups,mirallBranch=v2.0.2,slave=SMASH Build #13835 succeeded in 23 min
Details
smashbox-on-docker-ci/DOCKER_IMAGE=ubuntu_oc_lamp-git,TEST_NAME=test_sharePropagationInsideGroups,mirallBranch=v2.0.2,slave=SMASH Build #13835 succeeded in 35 min
Details

@DeepDiver1975 DeepDiver1975 deleted the fix-problems-caused-by-updater branch Apr 21, 2016

@VicDeo

This comment has been minimized.

Show comment
Hide comment
@VicDeo

VicDeo Apr 22, 2016

Member

Tested
Updating 9.0 to ownCloud 9.0.1 (daily) Build:2016-04-22T03:03:05+00:00 6365851
sudo -u www-data php occ integr:check-core
passed with no errors

Member

VicDeo commented Apr 22, 2016

Tested
Updating 9.0 to ownCloud 9.0.1 (daily) Build:2016-04-22T03:03:05+00:00 6365851
sudo -u www-data php occ integr:check-core
passed with no errors

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment