-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
[FIX][mail_environment][server_environment] Prevent ImportError. #666
[FIX][mail_environment][server_environment] Prevent ImportError. #666
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
small typo otherwise you get my 👍
try: | ||
from . import env_mail | ||
except ImportError: | ||
_logger.info("ImportError raised while loading module.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could be nice to tell which module is not able to be imported!
suggested:
_logger.info("ImportError raised while loading env_mail module.")
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need, the logs already say where do error come from, that's implicit by getLogger(__name__)
try: | ||
from .serv_config import serv_config, setboolean | ||
except ImportError: | ||
_logger.info("ImportError raised while loading module.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same comment here!
In fact the real matter is the missing dependency(sever_environement_files) for server_environment, isn't it? |
But AFAICS that addon should be created manually in each environment. OTOH, the real problem is not controlling the ImportError. That's a bug. |
ok thanks for those informations! i've never used this module :) I was reading that on the history on other PR... |
Me neither, but this bug is messing up other repos' bots... 😆 |
@yajo sorry for nagging, but you shouldn't merge your own PRs. |
the hard import: https://github.com/OCA/server-tools/blob/8.0/server_environment/serv_config.py#L32 |
What about use the new standard of ImportError to know the exactly package with the error? https://github.com/OCA/maintainer-tools/blob/master/CONTRIBUTING.md#importerror try:
import external_dependency_python_N
import external_dependency_python_M
EXTERNAL_DEPENDENCY_BINARY_N_PATH = tools.find_in_path('external_dependency_binary_N')
EXTERNAL_DEPENDENCY_BINARY_M_PATH = tools.find_in_path('external_dependency_binary_M')
except (ImportError, IOError) as err:
_logger.debug(err) |
Hi @dreispt, this had already 3x👍; isn't that enough to merge? (Sorry if I missed something, I've been PSC for not so long). @DarioLodeiros, I tried that. The problem is that if you catch the ImportError but still continue executing the whole module, another exception would be probably raised because the module was not loaded properly. I decided to catch the exception at a higher level to abort loading the rest of the file(s) if the import fails. After all, this is a quick patch for an addon that I don't use but breaks my environment, its actual developers should do a better patch if they need it 😉. @moylop260 If I understand fine, you mean that you want to get the ImportError traceback. Actually I'm using a feature of the logging module, the |
@yajo why was this module suddenly creating problems? It had not been causing any problem for years. |
No idea, but it happened. |
The source of the problem is the hard import of a module that does not exist... server_environment_files |
Indeed. I mean I don't know how come it was not failing before 😁 |
#617 created some noise because that module had a |
Nice to know that, although could you please detail what did I harm here? |
Syncing from upstream OCA/server-tools (12.0)
Fixes #488.
It should fix Travis in OCA/website, like in OCA/website#303 (comment).
@Tecnativa