Skip to content
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

Improve OS migration script #251

Merged
merged 8 commits into from
Apr 6, 2021
Merged

Conversation

njohner
Copy link
Contributor

@njohner njohner commented Feb 19, 2021

  • We allow setting new permissions on existing repository folders. Note that we only do this if inheritance is blocked and local_roles are indicated in the Excel. These will replace the existing sharing permissions
  • We skip the docproperties update
  • We add commits during the migration, this is a test to see whether it will make the migration faster.
  • I also improved logging a bit.

Tests were also updated, see 4teamwork/opengever.core#6792

For https://4teamwork.atlassian.net/browse/CA-1472

@njohner njohner requested a review from a team February 19, 2021 11:30
@lukasgraf lukasgraf self-assigned this Feb 22, 2021
Copy link
Contributor

@lukasgraf lukasgraf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍 , except the commit() not respecting the dryrun flag.

@@ -1013,6 +1013,7 @@ def create_repository_folders(self, items):
self.start_bundle_import(tmpdirname)

shutil.rmtree(tmpdirname)
transaction.commit()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These should respect options.dryrun.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True, thanks!

@lukasgraf lukasgraf removed their assignment Feb 22, 2021
As we anyway do the migration on a copy of the deployment, we
can take the risk of committing during the migration to speed
it up.
@njohner njohner force-pushed the nj/CA-1472/os_migration_permissions branch from 5767f0a to 98c9501 Compare February 22, 2021 12:30
@njohner
Copy link
Contributor Author

njohner commented Feb 22, 2021

All done. As discussed, the last commit will be tested in the next test-migration, see if it really speeds the migration up.

@njohner njohner requested a review from lukasgraf February 22, 2021 14:29
Copy link
Contributor

@lukasgraf lukasgraf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

This would access the blobs, which we do not have during
the migration.
@njohner
Copy link
Contributor Author

njohner commented Feb 24, 2021

@lukasgraf can you have a look at the last commit? I had to exclude reindexing the SearchableText as we won't have the blobs during migration.

@njohner njohner requested a review from lukasgraf February 24, 2021 15:13
Copy link
Contributor

@lukasgraf lukasgraf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

@njohner njohner merged commit d858202 into master Apr 6, 2021
@njohner njohner deleted the nj/CA-1472/os_migration_permissions branch April 6, 2021 08:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants