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

This allows you to raise a warning if dependencies are not unmet #39

Open
wants to merge 2 commits into
base: 8.0
Choose a base branch
from

Conversation

Danisan
Copy link

@Danisan Danisan commented Mar 15, 2016

Could see that raise fully stops the system and to error 500

@Yenthe666
Copy link
Owner

@Danisan thanks for the contribution, this looks neat!
Did you write this because you saw this? #38
I'm going to ask grexe to test this and give his feedback too, if it works fine from the terminal too I will merge it in.

@grexe
Copy link

grexe commented Mar 23, 2016

I'd prefer the error to be logged instead of silently pass'ed, though.
From the code I can see it should not cause any issues...

@grexe
Copy link

grexe commented Mar 23, 2016

hmmm still have no luck installing this module on 9.0 nightly though, while other modules worked.
Can't find any difference, folder and permissions are ok, using the default addons path
/usr/lib/python2.7/dist-packages/openerp/addons/

@Danisan
Copy link
Author

Danisan commented Jun 4, 2016

@grexe, with this you will have a more friendly warning about the lack of dependencies. @Yenthe666 actually I wasn't following the issues here, just a customer of mine installed the module and got fightened when everything was broken for him.

@gaikaz
Copy link

gaikaz commented Jan 9, 2019

This is a very good forgotten PR that is still relevant even in later versions of Odoo.
I only have one suggestion (something that @grexe already suggested in his issue #38). Instead of passing, _logger can be initialized above the try/except block and used to log the same error from currently raised ImportError. This would give more information in the logs for the sys. admin like the raise, yet won't stop further code execution like the pass.
I already tested this approach on Odoo v10.

@Yenthe666
Copy link
Owner

@gaikaz thanks for the feedback. I don't feel like doing pass in an Exception makes sense though. It feels like silently covering up a serious problem. Without the dependency the module wouldn't work for remote backups anyways so should we really allow a silent pass?

@gaikaz
Copy link

gaikaz commented Jan 9, 2019

@Yenthe666 , sorry, I wasn't very clear. I fully agree and this is exactly what I suggest changing to make this PR perfect. :)
Replace

raise ImportError('This module needs pysftp to automaticly write backups to the FTP through SFTP. Please install pysftp on your system. (sudo pip install pysftp)')

with

_logger.error('This module needs pysftp to automaticly write backups to the FTP through SFTP. Please install pysftp on your system. (sudo pip install pysftp)')

instead of pass
So it logs the error but doesn't stop the code like raise does.

@Yenthe666
Copy link
Owner

Yeah that is absolutely better but what if you're not looking at logs though? You wouldn't notice that you're missing a dependency. As the scheduler itself also runs in the background you might end up with a scheduler that keeps failing and you thinking that the backup module does not work though? While you're just missing a dependency.

I'm not sure if this warns the user enough. On the other hand, if you only backup locally (why though?) you would not need the extra dependency. In this case this example would then be better again.

@gaikaz
Copy link

gaikaz commented Jan 9, 2019

In this PR @Danisan also adds external_dependencies parameter in the manifest file in commit 5b518fc . It prevents installing the module if the dependency is missing (gives a very nice prompt). So you won't even reach the point, where you have a scheduler inserted.

@Yenthe666
Copy link
Owner

Aha I see! I'll test it soon, thanks. 👍

@Yenthe666 Yenthe666 self-requested a review January 9, 2019 15:59
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.

4 participants