-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Before installing new shipped apps we need to load authentication apps #22584
Conversation
By analyzing the blame information on this pull request, we identified @PVince81, @LukasReschke and @icewind1991 to be potential reviewers |
// update all shipped apps | ||
$disabledApps = $this->checkAppsRequirements(); | ||
$this->doAppUpgrade(); | ||
|
||
// install new shipped apps on upgrade | ||
OC_Installer::installShippedApps(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't find code that runs eventual code for shipped apps, that just need an update. Or aren't they installed then?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Never code too late in the night ..... updated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know it - was just a gut feeling.
d51ef3d
to
7bf658a
Compare
And assume that they are still working 🙊 I'd say we should change the order here? First update the apps, then enable authentication apps and then install the others? Otherwise when ldap or others need to be updated themselves, they might run into a problem here, because they are executed before they are updated themselves? |
And since we disable 3rdparty apps on the update, this would kick any non-default authentication app and the users would not be upgraded again? |
this is a pain ... we need to address the central user account table in 9.1 @karlitschek @cmonteroluque just FYI once more |
agreed |
Ref: central user account table: #21282 |
👍 in the 🙈 mode, it's better then before but still has potential to fail I guess |
@@ -334,6 +331,9 @@ private function doUpgrade($currentVersion, $installedVersion) { | |||
// upgrade appstore apps | |||
$this->upgradeAppStoreApps($disabledApps); | |||
|
|||
// install new shipped apps on upgrade | |||
OC_App::loadApps('authentication'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also all other usages indicate OC_App::loadApps(array('authentication'))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(alternatively we should adjust the PHPDoc if that is supposed to work as well)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will loadApps
automatically update these authentication apps too if needed ? (I don't remember)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
load will not, but that is why we do the upgrades before, see above
@owncloud/qa please add contact and calendar migration for ldap users - THX |
@LukasReschke PHPDoc addressed |
Calendar doesn't seem to work with ldap users after the upgrade.
Local user (in this case admin) works fine:
The data has been migrated, but seems to use have issue with the ldap user's id:
|
The same happens with contacts.
Notice that the url to access is diferent: contacts has a "users" part in the url that calendar doesn't have
|
Okay - looks like we have some issues with the new dav endpoints with respect to LDAP auth - @blizzz can I ask you to have a look? Do I miss some username convertion? @jvillafanez the issue of this PR is that upgrades for ldap users have not been executed properly - did this work out now here? THX a olot |
The upgrade went fine, and I don't see any relevant error in the logs. There is a lot of noise with the ldap.
Ldap users (both old and new) can enter ownCloud without problem after the upgrade. If there is anything more specific to check, ping me |
The addressbooks and calendars need to be copied over from the old tables to the new. |
All of those tables contains data. I can recognize data that I input in 8.2.2 in some of those tables. The only exception is the I think DB is fine |
this is not possible afaik THX a lot - so this looks good to me |
@jvillafanez did you test this in a docker? If so please tell me which one so I can look up some things. THX |
@jvillafanez do the issues stated in #22584 (comment) and #22584 (comment) still persist? |
tested by bug reporter: #22508 (comment) |
Before installing new shipped apps we need to load authentication apps
This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
fixes #22508