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

[16.0][MIG] database_cleanup: Migration to 16.0 #2684

Merged
merged 57 commits into from
Dec 29, 2023
Merged

[16.0][MIG] database_cleanup: Migration to 16.0 #2684

merged 57 commits into from
Dec 29, 2023

Conversation

miikanissi
Copy link
Contributor

Original PR author seems to be unavailable at #2473

This PR continues the progress made in that and includes a cherry pick to fix tests

Copy link
Member

@bealdav bealdav left a comment

Choose a reason for hiding this comment

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

LGTM

I've tested to manually add an ir.model.data and db_cleanup allow to purge nicely

Thanks for this PR

((modules - installed) + (modules - installed).downstream_dependencies()).write(
{"state": "to remove"}
)
installed.button_immediate_uninstall()
Copy link
Contributor

Choose a reason for hiding this comment

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

this code is complex and not obvious about the intent.

If I read correctly, modules not in installed, to upgrade (filtered here) and uninstallable, installed, to remove (default argument of downstream_dependencies) are excluded.
So it let: uninstalled to remove, to install. Is it expected ?

Plus, it looks like this code can be shorter if it use the right exclude_filter of modules.downstream_dependencies().

Copy link
Contributor

Choose a reason for hiding this comment

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

@miikanissi Any comment on above question?

Copy link
Member

@hbrunn hbrunn Dec 23, 2023

Choose a reason for hiding this comment

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

I asked to add this in the previous PR: hbrunn@08c5df6

That's because in v16 a test was added in this function that raises for modules that are not installed.

[cut some nonsense I wrote before]

Copy link
Contributor

@robyf70 robyf70 left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@robyf70 robyf70 left a comment

Choose a reason for hiding this comment

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

LGTM

@ChrisOForgeFlow
Copy link
Contributor

/ocabot rebase

@OCA-git-bot
Copy link
Contributor

@ChrisOForgeFlow The rebase process failed, because command git merge tmp-pr-2684 failed with output:

Auto-merging .github/workflows/test.yml
CONFLICT (content): Merge conflict in .github/workflows/test.yml
Auto-merging .copier-answers.yml
Automatic merge failed; fix conflicts and then commit the result.

@ChrisOForgeFlow
Copy link
Contributor

@miikanissi Can you rebase and fix conflicts, thank you in advance

Stefan Rijnhart and others added 21 commits October 30, 2023 13:35
avoid ''NoneType' object has no attribute 'exists'' error when purging models
fix my change guewen.baconnier@camptocamp.com-20140203103254-v1mzu2uib047xb9h, wrong lines replaced...
    raw SQL query (but never read afterwards). Workaround for
    lp:1277899

[FIX] Preserve dangling workflow table which is in use

[RFR] Group models per table when detecting columns to purge
      to prevent problems with models sharing the same table

[ADD] Allow purging of dangling data entries

[FIX] Data purging now working

[IMP] Docstrings

[FIX] Label
[FIX] Catch attempt to unlink field from nonexisting model

[RFR] Flake8
[CHG] database_cleanup: move description to README.rst
[IMP] order wizard lines by name

[IMP] deal with modules whose models can't be loaded

[IMP] double quotes for docstring

[FIX] use exists query instead of huge in list

[IMP] hide unnecessary buttons in wizard II

[IMP] readability

[FIX] cope with purging nonexisting models
[ADD] test purging modules

[ADD] test purging tables
[FIX] database_cleanup reloads the registry

which has weird side effects during testing. Take care
database_cleanup's tests don't mess up the following tests
* [FIX] database_cleanup: Isolate build
* Isolate `database_cleanup` into its own build in Travis file to fix #689

* [FIX] database_cleanup: Remove KeyError assertion
* Remove KeyError assertion in tests due to PR in comment being merged
* [ADD] allow creating missing indexes

* [FIX] tests; installation

* [ADD] allow purging properties

* [ADD] missing file

* [ADD] test purging properties

* [ADD] missing parent_id for menu entry

* [FIX] don't delete too many and wrong properties
Using new base model inheritance.
[FIX] don't try to uninstall uninstalled modules

[DEL] weird code

[FIX] actually cleanup where we can
database_cleanup/tests/common.py Outdated Show resolved Hide resolved
@hbrunn
Copy link
Member

hbrunn commented Dec 24, 2023

please also include #2648 here

@miikanissi
Copy link
Contributor Author

please also include #2648 here

Included and rebased. Good to go?

@hbrunn
Copy link
Member

hbrunn commented Dec 28, 2023

Thanks for the followup.

Still need to deal with the last test failure

I'd suggest to patch away self.env.cr.commit in the test using self.patch

Or maybe we don't need the commit in v16?

@miikanissi
Copy link
Contributor Author

I'm not very familiar with the unittest mock objects. I'd need help to figure out the patch.

@hbrunn
Copy link
Member

hbrunn commented Dec 29, 2023

off the top of my head: self.patch(self.env.cr, 'commit', lambda: None)

@miikanissi
Copy link
Contributor Author

off the top of my head: self.patch(self.env.cr, 'commit', lambda: None)

@hbrunn That patch did work correctly and allowed the test to continue. However as stated here, not committing seems to cause the ORM to hang indefinitely.

However I cherry picked 00cdfb2 from 15.0 which improved on the tests

@pedrobaeza
Copy link
Member

/ocabot merge nobump

@OCA-git-bot
Copy link
Contributor

This PR looks fantastic, let's merge it!
Prepared branch 16.0-ocabot-merge-pr-2684-by-pedrobaeza-bump-nobump, awaiting test results.

@OCA-git-bot
Copy link
Contributor

This PR has the approved label and has been created more than 5 days ago. It should therefore be ready to merge by a maintainer (or a PSC member if the concerned addon has no declared maintainer). 🤖

1 similar comment
@OCA-git-bot
Copy link
Contributor

This PR has the approved label and has been created more than 5 days ago. It should therefore be ready to merge by a maintainer (or a PSC member if the concerned addon has no declared maintainer). 🤖

@OCA-git-bot OCA-git-bot merged commit 3594697 into OCA:16.0 Dec 29, 2023
9 checks passed
@OCA-git-bot
Copy link
Contributor

Congratulations, your PR was merged at c963e27. Thanks a lot for contributing to OCA. ❤️

@pedrobaeza
Copy link
Member

/ocabot migration database_cleanup

@OCA-git-bot OCA-git-bot added this to the 16.0 milestone Dec 29, 2023
@OCA-git-bot OCA-git-bot mentioned this pull request Dec 29, 2023
33 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.