Skip to content

Conversation

@metaminux
Copy link

…to run at install too

Fixes #4106

@OCA-git-bot
Copy link
Contributor

Hi @hbrunn, @StefanRijnhart, @legalsylvain,
some modules you are maintaining are being modified, check this out!

Copy link
Member

@hbrunn hbrunn left a comment

Choose a reason for hiding this comment

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

thanks, the CI errors are unrelated.

do we have any core modules where this is the case in v16? If so, a test for that would be great.

@metaminux
Copy link
Author

@hbrunn
Copy link
Member

hbrunn commented Aug 22, 2023

never tried it, but I think it could just work to drop a folder with the module name and correct version into https://github.com/OCA/OpenUpgrade/tree/16.0/openupgrade_scripts/scripts, cf https://github.com/OCA/OpenUpgrade/blob/16.0/openupgrade_framework/odoo_patch/odoo/tests/loader.py#L30

Edit: ah wait, those are localization modules and those aren't installed in the test database we use. And it must be a newly pulled module anyways. How about adding an end migration script for ie spreadsheet (that's new in v16 and pulled during the migration), check for environment variable OPENUPGRADE_TESTS and if that's set, ie set some config parameter as marker that you can check for in the test?

@metaminux
Copy link
Author

Hello @hbrunn

Instead of creating an end-migration script in the "production" tree of openupgrade_scripts, I'd like to copy such script during the tests using data.py file... but I'm struggling to get a right path from the Odoo shell.

It seems I can't get the value of GITHUB_WORKSPACE... any advice ?

@metaminux metaminux force-pushed the 16.0-openupgrade_framework_end_mig_at_end branch 2 times, most recently from 070a19a to 16de61e Compare August 29, 2023 14:11
@hbrunn
Copy link
Member

hbrunn commented Aug 29, 2023

just $GITHUB_WORKSPACE is empty? Not sure however what I think of this approach, anyone else an opinion?

Just to be clear, I don't insist on the test, just thought it was nice

@pedrobaeza
Copy link
Member

No idea, sorry.

@legalsylvain legalsylvain added this to the 16.0 milestone Aug 31, 2023
@hbrunn
Copy link
Member

hbrunn commented Dec 23, 2023

so... do we go ahead and merge this?

@pedrobaeza
Copy link
Member

OK, please rebase to check the CI.

@metaminux
Copy link
Author

H everyone,

CI is not OK after rebase, I'll convert this PR to draft for the moment because it seems to me that some others changes in the way odoo is loading modules are preventing post-* and end-* migration scripts to execute for newly installed ones...

@metaminux metaminux marked this pull request as draft January 9, 2024 11:36
@metaminux metaminux marked this pull request as ready for review January 22, 2025 10:47
@metaminux
Copy link
Author

Hi @hbrunn Hi @pedrobaeza

Sorry for the delay... I just rebased and had to fix some typo for pre-commit to run OK.

I think we can go ahead...
Tests are using the end migration script for spreadsheet which is newly installed. See https://github.com/OCA/OpenUpgrade/actions/runs/12905959373/job/35986260407?pr=4107#step:16:18575

@MiquelRForgeFlow
Copy link
Contributor

Could you rebase correctly? I see merge branch commits there. I think you should have here only two commits (the pre-commit commit can be fused with the commit that adds tests)

@metaminux metaminux force-pushed the 16.0-openupgrade_framework_end_mig_at_end branch from 971df15 to c2f89a6 Compare January 22, 2025 14:16
@metaminux
Copy link
Author

@MiquelRForgeFlow Rebase correctly done, thanks

end migration script for newly installed spreadsheet module runs : https://github.com/OCA/OpenUpgrade/actions/runs/12910128466/job/35999439017?pr=4107#step:16:18582

to_install = pkg.state == "to install"
if to_install:
pkg.state = "to upgrade"
load_state_to_install = getattr(pkg, "load_state", False)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you should adapt the docstring of this method...

@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). 🤖

@MiquelRForgeFlow MiquelRForgeFlow added the no stale Use this label to prevent the automated stale action from closing this PR/Issue. label Mar 13, 2025
@@ -0,0 +1,14 @@
# Copyright 2023 Odoo Community Association (OCA)
Copy link
Member

Choose a reason for hiding this comment

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

This is not testing really the feature, as this module is not installed during the upgrade.

Copy link
Author

Choose a reason for hiding this comment

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

Hi @pedrobaeza

Look at .github/workflows/test.yml, this file is copied to spreadsheet directory which is installed during the upgrade.

Do you think I should create a directory containing only tests ?

Copy link
Member

Choose a reason for hiding this comment

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

OK, I see, it seems it works for this version, but for the next ones, spreadsheet will be already installed.

@pedrobaeza
Copy link
Member

/ocabot merge patch

@OCA-git-bot
Copy link
Contributor

What a great day to merge this nice PR. Let's do it!
Prepared branch 16.0-ocabot-merge-pr-4107-by-pedrobaeza-bump-patch, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit 380824d into OCA:16.0 Jul 15, 2025
2 checks passed
@OCA-git-bot
Copy link
Contributor

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

@metaminux metaminux deleted the 16.0-openupgrade_framework_end_mig_at_end branch July 15, 2025 08:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved merged 🎉 no stale Use this label to prevent the automated stale action from closing this PR/Issue. ready to merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[16.0] openupgrade_framework : patched method migrate_module should change load_state for end-migration scripts to be run at install

6 participants