-
Notifications
You must be signed in to change notification settings - Fork 76
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
fix(Groups): take over members during migration #863
Conversation
53effdd
to
7762d3b
Compare
Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>
7762d3b
to
6063847
Compare
Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>
dba2f40
to
e2604db
Compare
Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>
…mbers Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>
Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>
541cfdd
to
69e423a
Compare
This comment was marked as resolved.
This comment was marked as resolved.
4de43a1
to
f5513a5
Compare
And I "disable" the app "user_saml" | ||
And the local group "Astrophysics" is created | ||
And I hack "student2" into existence | ||
And the user "student2" is added to the group "Astrophysics" | ||
And I "enable" the app "user_saml" |
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.
This is plain awful, but I did not come up with any other way to resemble this situation. Easier would be issuing direct SQL queries, however this would be written again and support all DB flavours, and detect what is running on which port.
aka "occ saml:group-migration:copy-incomplete-members" Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>
f5513a5
to
244abb4
Compare
$this->groupMigration->cleanUpOldGroupUsers($gid); | ||
} catch (Exception $e) { | ||
$this->logger->warning('Error while cleaning up group members in (oc_)group_user of group (gid) {gid}', [ | ||
'app' => 'user_saml', |
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.
Isn't the app already set when getting the LoggerInterface through DI?
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.
Isn't the app already set when getting the LoggerInterface through DI?
I am never sure of that 🙈 Sometimes you see logs without an app set.
fixes #862
maybe add an occ command to see pending migrationshave a repair step insteadAlso contains a drive-by fix. I noticed that the list of groups is set newly on every repair step run. Commit 541cfdd fixes this.
failing test proof