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

[MIG] openupgrade_framework, base v17 #4427

Merged
merged 40 commits into from
May 23, 2024

Conversation

royle-viindoo
Copy link
Contributor

This is preliminary because I didn't yet go through all analysis to migrate them, but this lets the base migration run without crashing

TODOs:

  • verify we still need all monkey patches and no new ones
  • mig base (partial)

legalsylvain and others added 25 commits May 13, 2024 15:25
If we don't include it, /jsonrpc route doesn't work.
If you have a module in previous versions that adds data on a model,
and such model is not loaded in the registry in current version because
the module is absent in it, you can't uninstall such module, getting
this error:

  File "odoo/odoo/addons/base/models/ir_model.py", line 1945, in _module_data_uninstall
    delete(self.env[model].browse(item[1] for item in items))
  File "odoo/odoo/api.py", line 463, in __getitem__
    return self.registry[model_name]._browse(self, (), ())
  File "odoo/odoo/modules/registry.py", line 177, in __getitem__
    return self.models[model_name]
KeyError: 'model'

With this patch, data cleanup of such model is skipped and there's no crash.
…ore ; add links to the new OpenUpgrade website
If corresponding field is None, we need to avoid the "AttributeError: 'NoneType' object has no attribute error.
…e useless after the input argument validate has been removed
@royle-viindoo royle-viindoo force-pushed the v17_mig_openupgrade_framework_base branch 4 times, most recently from 33e5b63 to a70fc46 Compare May 15, 2024 04:02
@duong77476-viindoo duong77476-viindoo force-pushed the v17_mig_openupgrade_framework_base branch 2 times, most recently from f729510 to 81bc5f3 Compare May 22, 2024 04:36
@duong77476-viindoo
Copy link
Contributor

Hi I have followed up some comments in #4327 by Mr @StefanRijnhart and comment #4427 (comment) by @pedrobaeza
Please continue review, thanks
I also split base migration into another pr #4430
and make a mail migration also at #4431

P/S: How can i add v16 test database
image

@StefanRijnhart
Copy link
Member

Can you help me here? I asked on #4327 to remove https://github.com/OCA/OpenUpgrade/blob/16.0/openupgrade_framework/odoo_patch/odoo/tests/loader.py, and it's certainly gone here but it's not removed in the migration commit. Where is it removed?

@StefanRijnhart
Copy link
Member

Please prefix the migration commit with [MIG], not [IMP]
image

@StefanRijnhart
Copy link
Member

16.0 reference database is now added to https://github.com/OCA/OpenUpgrade/releases/tag/databases.

@duong77476-viindoo
Copy link
Contributor

Can you help me here? I asked on #4327 to remove https://github.com/OCA/OpenUpgrade/blob/16.0/openupgrade_framework/odoo_patch/odoo/tests/loader.py, and it's certainly gone here but it's not removed in the migration commit. Where is it removed?

I drop that commit (using git rebase and drop) outside of this PR, so you mean that i need to keep it and make another commit to remove it?

@duong77476-viindoo duong77476-viindoo force-pushed the v17_mig_openupgrade_framework_base branch from 81bc5f3 to d22c24f Compare May 23, 2024 06:12
@duong77476-viindoo
Copy link
Contributor

16.0 reference database is now added to https://github.com/OCA/OpenUpgrade/releases/tag/databases.

Thank you

@duong77476-viindoo duong77476-viindoo force-pushed the v17_mig_openupgrade_framework_base branch from d22c24f to ed631af Compare May 23, 2024 06:31
@duong77476-viindoo
Copy link
Contributor

Please prefix the migration commit with [MIG], not [IMP] image

Done

@StefanRijnhart
Copy link
Member

Can you help me here? I asked on #4327 to remove https://github.com/OCA/OpenUpgrade/blob/16.0/openupgrade_framework/odoo_patch/odoo/tests/loader.py, and it's certainly gone here but it's not removed in the migration commit. Where is it removed?

I drop that commit (using git rebase and drop) outside of this PR, so you mean that i need to keep it and make another commit to remove it?

Yes please! You can remove the code in the migration commit. That way, we keep neatly track of the changes per version.

@duong77476-viindoo
Copy link
Contributor

@StefanRijnhart OK, then can i cherry-pick that commit then i'll create a commit to remove .Meaning that the test commit and its removal will come on top. Is it ok ?

@StefanRijnhart
Copy link
Member

@duong77476-viindoo Actually, that's not conventional. Please back-up this branch to another name (can be locally), then reset the branch and redo the git format-patch method with which this branch was created in the first place. Then cherry-pick your migration commit, then amend the migration commit.

@duong77476-viindoo duong77476-viindoo force-pushed the v17_mig_openupgrade_framework_base branch from ed631af to 358ece8 Compare May 23, 2024 08:20
@duong77476-viindoo
Copy link
Contributor

@StefanRijnhart Hello, i have done what you advised, please review, thanks

-This commit remove unecessary patch
-Check if test exist then execute it in the test.yml
-Remove the test loader from patch, detail at
OCA#4327 (comment)
@duong77476-viindoo duong77476-viindoo force-pushed the v17_mig_openupgrade_framework_base branch from 358ece8 to 52de874 Compare May 23, 2024 08:35
Copy link
Member

@StefanRijnhart StefanRijnhart left a comment

Choose a reason for hiding this comment

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

Nice work!

@pedrobaeza pedrobaeza added this to the 17.0 milestone May 23, 2024
@pedrobaeza pedrobaeza merged commit ed43b0d into OCA:17.0 May 23, 2024
2 checks passed
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.

None yet

9 participants