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

[ADD] oca_checklog_odoo: configurable failure on WARNING log messages #77

Merged
merged 2 commits into from
Sep 30, 2024

Conversation

PabloEForgeFlow
Copy link
Contributor

This PR implements #10 by doing the following:

  • Add bin/oca_checklog_odoo, which fails on stdin WARNING log messages when OCA_ENABLE_CHECKLOG_ODOO is set
  • Pipe oca_init_test_database and oca_run_tests to oca_checklog_odoo
  • Test functionality in tests/test_checklog.py

@sbidoul
Copy link
Member

sbidoul commented Sep 16, 2024

I've had a brief look, and the PR looks great.

Regarding the python 3.5 test, well... I wonder if OCA should not simply stop supporting Odoo 11.

if [ -n "${OCA_ENABLE_CHECKLOG_ODOO}" ]; then
checklog-odoo
else
checklog-odoo -i ".*"
Copy link
Member

Choose a reason for hiding this comment

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

I know I proposed this approach, but I wonder if we could use a simple unix command instead such as cat?

Copy link
Member

Choose a reason for hiding this comment

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

I mean, specifically for the case when we don't want to check logs

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, that would probably make more sense. Changed it just now

Copy link

@hparfr hparfr left a comment

Choose a reason for hiding this comment

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

minor remark about typo

README.md Outdated Show resolved Hide resolved
@PabloEForgeFlow
Copy link
Contributor Author

I've had a brief look, and the PR looks great.

Regarding the python 3.5 test, well... I wonder if OCA should not simply stop supporting Odoo 11.

I've had a brief look, and the PR looks great.

Regarding the python 3.5 test, well... I wonder if OCA should not simply stop supporting Odoo 11.

Apparently it's been almost a decade since 3.5 was released, so yeah it might be time to let it go

Copy link

@rousseldenis rousseldenis left a comment

Choose a reason for hiding this comment

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

LGTM

@sbidoul
Copy link
Member

sbidoul commented Sep 25, 2024

I'll try to find a moment to test this soon. Main thing I want to check is if the colors are preserved in the logs.

@PabloEForgeFlow would you also do the PR on oca/oca-addons-repo-template to add the copier question to enable this (enabled by default on Odoo >= 18)?

@PabloEForgeFlow
Copy link
Contributor Author

I'll try to find a moment to test this soon. Main thing I want to checks is if the colors are preserved in the logs.

@PabloEForgeFlow would you also do the PR on oca/oca-addons-repo-template to add the copier question to enable this (enabled by default on Odoo >= 18)?

Sure, I'm a bit busy at the moment, but I'll do that as soon as I can.

@PabloEForgeFlow
Copy link
Contributor Author

PabloEForgeFlow commented Sep 26, 2024

What do you think about reusing the github_ci_extra_env variable? OCA/oca-addons-repo-template#271

Edit: Just realized I did not take the Travis configuration into account, perhaps it is worth having a dedicated variable.

@sbidoul
Copy link
Member

sbidoul commented Sep 26, 2024

Don't worry about Travis. We don't use it anymore (and I think we can remove it from the template).

About github_ci_extra_env that could work, but we need a default value for 18+ that enables the checklog feature, so it is applied by default when creating new branches with oca/repo-maintainer-conf. A new question would be friendlier, though.

@sbidoul
Copy link
Member

sbidoul commented Sep 30, 2024

Ok, lets merge it to test.

@sbidoul sbidoul merged commit 4d42993 into OCA:master Sep 30, 2024
16 of 18 checks passed
@sbidoul
Copy link
Member

sbidoul commented Sep 30, 2024

Thanks a lot for doing this @PabloEForgeFlow

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants