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

[MIG] base_rest, base_rest_auth_api_key, base_rest_datamodel, base_rest_demo, base_rest_pydantic, datamodel, extendable: Migration to 16.0 #313

Merged
merged 6 commits into from
Jan 26, 2023

Conversation

StefanRijnhart
Copy link
Member

@StefanRijnhart StefanRijnhart commented Dec 4, 2022

Started out from #312. I assigned co-authorship of the main migration commit to @Nikul-OSI.

@StefanRijnhart StefanRijnhart added this to the 16.0 milestone Dec 4, 2022
@lmignon
Copy link
Contributor

lmignon commented Dec 6, 2022

@StefanRijnhart I don't plan to continue to work with base_rest for my own projects (at least at this stage). I'll finalize the integration with fastapi since this new addon add a lot benefits over base_rest.

@StefanRijnhart StefanRijnhart force-pushed the 16.0-mig-base_rest branch 4 times, most recently from b7b1db2 to 6cbef43 Compare December 7, 2022 15:24
@StefanRijnhart StefanRijnhart changed the title [WIP] Migration of base_rest and friends to 16.0 [MIG] base_rest, base_rest_auth_api_key, base_rest_datamodel, base_rest_demo, base_rest_pydantic, datamodel, extendable: Migration to 16.0 Dec 7, 2022
@StefanRijnhart
Copy link
Member Author

StefanRijnhart commented Dec 7, 2022

@lmignon thanks for commenting. I'm adding a warning about that during the loading of base_rest. I thought I'd remove you as the maintainer of the module as well but let me know if you like to keep it.
We'll probably migrate to your fastapi integration when it's ready but we need something to get started on for Odoo 16.

@lmignon
Copy link
Contributor

lmignon commented Dec 7, 2022

You can remove me as maintainer even if I could still contribute if we need it for legacy code. Into the migration you could also remove all the process that transform the code from the old api to the new one. (A warning has been added in 14.0 #270 to notify that the implicit mode is deprecated)

@StefanRijnhart StefanRijnhart force-pushed the 16.0-mig-base_rest branch 3 times, most recently from ba32617 to 3b6faf0 Compare December 7, 2022 16:11
@StefanRijnhart
Copy link
Member Author

@lmignon at this point removing obsolete code from a deprecated module does not seem worth the trouble ;-)

@StefanRijnhart StefanRijnhart marked this pull request as ready for review December 7, 2022 16:13
@OCA-git-bot OCA-git-bot mentioned this pull request Dec 7, 2022
14 tasks
@OCA OCA deleted a comment from OCA-git-bot Dec 7, 2022
@StefanRijnhart StefanRijnhart force-pushed the 16.0-mig-base_rest branch 2 times, most recently from d68edd3 to c496fb1 Compare December 7, 2022 16:24
…st_demo, base_rest_pydantic, datamodel, extendable: pre-commit stuff
@StefanRijnhart
Copy link
Member Author

@lmignon thanks, I added another test now with a different parameter than id.

Copy link
Contributor

@lmignon lmignon left a comment

Choose a reason for hiding this comment

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

Thank you @StefanRijnhart LGTM (Code review)

@StefanRijnhart
Copy link
Member Author

Thanks for the review! I squashed the fixup commits.

@lmignon
Copy link
Contributor

lmignon commented Jan 23, 2023

@marielejeune @qgroulard Can you make a test with this RP. I have in mind that you have modules to migrate in 16 that are based on base_rest. It would be great to have an additional functional review/test.

Copy link
Member

@FrancoMaxime FrancoMaxime left a comment

Choose a reason for hiding this comment

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

LGTM & Functional review

base_rest_demo/__manifest__.py Show resolved Hide resolved
"development_status": "Beta",
"license": "LGPL-3",
"author": "ACSONE SA/NV, " "Odoo Community Association (OCA)",
"maintainers": ["lmignon"],
Copy link
Member

@FrancoMaxime FrancoMaxime Jan 23, 2023

Choose a reason for hiding this comment

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

@lmignon, as you requested you have been removed from the list of maintainers for this module. Maybe you would like to be removed from other modules like base_rest_demo?

@StefanRijnhart
Copy link
Member Author

@HoangSang1510, kindly reminding you to test the latest version of this code.

Copy link
Contributor

@marielejeune marielejeune 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 the work! LGTM (functional tests)

@lmignon
Copy link
Contributor

lmignon commented Jan 26, 2023

/ocabot merge nobump

@OCA-git-bot
Copy link
Contributor

This PR looks fantastic, let's merge it!
Prepared branch 16.0-ocabot-merge-pr-313-by-lmignon-bump-nobump, awaiting test results.

@lmignon
Copy link
Contributor

lmignon commented Jan 26, 2023

Thank you to all reviewers and contributors (especially @StefanRijnhart) ! It's time to merge this huge work.

@OCA-git-bot OCA-git-bot merged commit 316aad7 into OCA:16.0 Jan 26, 2023
@OCA-git-bot
Copy link
Contributor

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

@StefanRijnhart
Copy link
Member Author

StefanRijnhart commented Jan 26, 2023

Thanks for all the reviews and to @Nikul-OSI for his part in the migration!

@HoangSang1510
Copy link

@HoangSang1510, kindly reminding you to test the latest version of this code.

I have updated to latest version, now it work perfectly in my project. Thank you very much!

@StefanRijnhart
Copy link
Member Author

@HoangSang1510 thanks for confirming!

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.

7 participants