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.0][FIX] connector_search_engine: missing typing-extensions external dependency #194

Merged
merged 1 commit into from
May 22, 2024

Conversation

clementmbr
Copy link
Member

@clementmbr clementmbr commented May 22, 2024

Trivial PR to fix this kind of bug in CI:
https://github.com/OCA/sale-channel/actions/runs/8332403108/job/22801435634?pr=17#step:7:446

This bug doesn't occur on search-engine repo because the module search_engine_serializer_pydantic use pydantic which depends on python package typing-extensions. So typing-extensions is loaded anyway.

But when a module on another repo like sale_channel needs to use connector_search_enginewithout using search_engine_serializer_pydantic, the bug occurs.

Once this PR is merged I will be able to merge OCA/sale-channel#17

cc @florian-dacosta @bealdav @rousseldenis @adrienpeiffer

@clementmbr clementmbr force-pushed the 16.0-fix-python-dependencies branch from 86b2271 to 43d2526 Compare May 22, 2024 13:52
Copy link
Contributor

@florian-dacosta florian-dacosta left a comment

Choose a reason for hiding this comment

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

Thanks for this fix
I don't know if the lib has to be added manually in requirement file, because it say it is "generated from manifests external_dependencies".

I am not sure though and I guess it won't hurt.

@rousseldenis
Copy link
Contributor

Thanks for this fix I don't know if the lib has to be added manually in requirement file, because it say it is "generated from manifests external_dependencies".

I am not sure though and I guess it won't hurt.

Pre-commit does the job for you if you add a dependency in manifest

@clementmbr
Copy link
Member Author

I don't know if the lib has to be added manually in requirement file, because it say it is "generated from manifests external_dependencies".

I didn't run pre-commit on my local branch, so I had to add the lib to the requirements manually as the pre-commit test went red here.

Anyway, @OCA/search-engine-maintainers is it possible to merge this trivial PR? Thanks!

Copy link
Member

@rvalyi rvalyi left a comment

Choose a reason for hiding this comment

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

@florian-dacosta @clementmbr just to make it clear: usually you run pre-commit locally and it adds the dependencies in requirements.txt, and you should commit them along with the change to make the CI happy. But adding it manually like @clementmbr did is perfectly fine as long as you put it at the right place so the requirements.txt generated by the CI matches it perfectly which is the case here.

@rvalyi
Copy link
Member

rvalyi commented May 22, 2024

/ocabot merge patch

@OCA-git-bot
Copy link
Contributor

On my way to merge this fine PR!
Prepared branch 16.0-ocabot-merge-pr-194-by-rvalyi-bump-patch, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit 9926e95 into OCA:16.0 May 22, 2024
7 checks passed
@OCA-git-bot
Copy link
Contributor

Congratulations, your PR was merged at e3585ba. 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.

6 participants