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][MIG ] module_analysis #2609

Merged
merged 30 commits into from
May 16, 2023
Merged

Conversation

florian-dacosta
Copy link
Contributor

@florian-dacosta florian-dacosta commented Apr 18, 2023

Replace #2608

Sorry I guess I did a wrong manipulation, I could not reopen the other after a force push.

@legalsylvain

legalsylvain and others added 28 commits April 18, 2023 14:38
fixup! [ADD] new module module_analysis

fixup! fixup! [ADD] new module module_analysis

fixup! fixup! fixup! [ADD] new module module_analysis

fixup! fixup! fixup! fixup! [ADD] new module module_analysis

IMP exception message

fixup! fixup! fixup! fixup! fixup! [ADD] new module module_analysis

[REF] remove use of cloc. use pygount librairy instead

fixup! [REF] remove use of cloc. use pygount librairy instead

fixup! fixup! [REF] remove use of cloc. use pygount librairy instead

Apply suggestions from code review

Co-Authored-By: David Beal <david.beal@akretion.com>

Update module_analysis/views/menu.xml

Co-Authored-By: David Beal <david.beal@akretion.com>

Update module_analysis/tests/test_module.py

Co-Authored-By: David Beal <david.beal@akretion.com>

Update module_analysis/readme/CONFIGURE.rst

Co-Authored-By: David Beal <david.beal@akretion.com>

[IMP] handle encoding

[UPD] Update module_analysis.pot

[UPD] README.rst

[UPD] README.rst
Currently translated at 100.0% (35 of 35 strings)

Translation: server-tools-12.0/server-tools-12.0-module_analysis
Translate-URL: https://translation.odoo-community.org/projects/server-tools-12-0/server-tools-12-0-module_analysis/zh_CN/

[UPD] README.rst
Currently translated at 100.0% (35 of 35 strings)

Translation: server-tools-13.0/server-tools-13.0-module_analysis
Translate-URL: https://translation.odoo-community.org/projects/server-tools-13-0/server-tools-13-0-module_analysis/it/
[MIG] module_analysis: Migration to 15.0
…cause the analysis is partial (it also make the update slower) ; Add instead a cron task that is executed nightly to update analysis automatically
Currently translated at 97.2% (35 of 36 strings)

Translation: server-tools-15.0/server-tools-15.0-module_analysis
Translate-URL: https://translation.odoo-community.org/projects/server-tools-15-0/server-tools-15-0-module_analysis/it/
Analyse can take some time and a cron task can do it every day if needed, no need to recompute it in real time
@florian-dacosta
Copy link
Contributor Author

@legalsylvain

Why 8e5457e

In general I prefer all cron to be unactive at installation, mainly this kind of cron that you have to do some configuration (better change the time to run it at night at least, etc). It does not change a lot to unactivate it because you have to configure it anyway.
Having active cron of the kind is annoying for test database, CI, preprod, etc...

You'll tell me if it is acceptable for you, else I'll remove the commit so we can go forward

@legalsylvain
Copy link
Contributor

I have no clear point of view.
In one hand, you removed the analysis in the post_install part, and you set active to false for the cron. So when we install the module, now, nothing "work", (and will work) and the configuration is now mandatory.
In the other hand, your argument are valid. ;-)

So, it's as you wish on that topic !
But if we go in that direction, you should rewrite some readme files, because some part are obsolete / wrong now.

@florian-dacosta
Copy link
Contributor Author

Thanks @legalsylvain I always forget to check the readme files...
I've updated it the readme file and I also added the possibility to add a custom domain in the cron, because we have a use case where we'd like the analysis to run on uninstalled modules. (I will allow to know the "cost" of your module before installing it...)


* Check the box 'Analyse Installed modules'
* Run manually the action : 'Update Module Analysis'

.. image:: ../static/description/base_module_update.png
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess that line has to be removed. (and the according png file).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've added the line => To run an update manually, you can run the scheduled action manually.
The image is indeed to remove

@legalsylvain
Copy link
Contributor

(I will allow to know the "cost" of your module before installing it...)

good idea !

… cron.

Usefull if we want to analyse the code for uninstalled modules for instance
Copy link
Contributor

@PierrickBrun PierrickBrun left a comment

Choose a reason for hiding this comment

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

looks good, I tested it in a customer project and the analysis worked well

@florian-dacosta
Copy link
Contributor Author

@legalsylvain
Would you mind updating your review ?

Copy link
Contributor

@legalsylvain legalsylvain left a comment

Choose a reason for hiding this comment

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

Thanks !

/ocabot merge nobump

@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
Copy link
Contributor

Sorry @legalsylvain you are not allowed to merge.

To do so you must either have push permissions on the repository, or be a declared maintainer of all modified addons.

If you wish to adopt an addon and become it's maintainer, open a pull request to add your GitHub login to the maintainers key of its manifest.

@bguillot
Copy link
Contributor

/ocabot merge nobump

@bguillot
Copy link
Contributor

Thanks !

@OCA-git-bot
Copy link
Contributor

Hey, thanks for contributing! Proceeding to merge this for you.
Prepared branch 16.0-ocabot-merge-pr-2609-by-bguillot-bump-nobump, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit f90e1b6 into OCA:16.0 May 16, 2023
@OCA-git-bot
Copy link
Contributor

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

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.

None yet