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

[15.0][MIG] base_sequence_option #2275

Merged
merged 6 commits into from
Nov 3, 2023

Conversation

Rad0van
Copy link
Contributor

@Rad0van Rad0van commented Feb 7, 2022

This would be a standard migration if only... I cannot force Odoo to run tests for this. Not in 15.0, not in 14.0 Anybody's got any idea why? So this is sort of an blind attempt based only on fact it seems to work together with account_sequence_option that I plan to port next. Had to add return statement here:

    @classmethod
    def tearDownClass(cls):
        cls.loader.restore_registry()
        return super(CommonBaseSequenceOption, cls).tearDownClass()

That's pretty strange because such code exists in Odoo itself (didn't look long enough to find such thing in OCA). Also had to remove string attribute from tree element in view definition as per OCA/oca-addons-repo-template#76 It was nagging as well.

Pylint was nagging me with
base_sequence_option/tests/common.py:91: [W8110(missing-return), CommonBaseSequenceOption.tearDownClass] Missing `return` (`super` is used) in method tearDownClass.

Any feedback welcome.

@Rad0van
Copy link
Contributor Author

Rad0van commented Feb 8, 2022

@kittiu would you have a look here please? Alsowhat does that ci/runboat failing at the beginning mean? @gurneyalex ?

@kittiu
Copy link
Member

kittiu commented Feb 9, 2022

I can not see any runbot log. Can you somehow try to push again. May be you can squash commits and try again.
I am not sure what it is but some time I experiences ci/runbot just behave like that.
But as other test are passing, I believe it is eligible for merge too, if reviewed.

@Rad0van Rad0van force-pushed the 15.0-mig-base_sequence_option branch from 5374355 to f6ffc28 Compare February 9, 2022 07:51
@Rad0van
Copy link
Contributor Author

Rad0van commented Feb 9, 2022

I can not see any runbot log. Can you somehow try to push again. May be you can squash commits and try again. I am not sure what it is but some time I experiences ci/runbot just behave like that. But as other test are passing, I believe it is eligible for merge too, if reviewed.

Even force-push didn't help. I cannot see anything there as well. Maybe @gurneyalex would know?

@Rad0van
Copy link
Contributor Author

Rad0van commented Feb 10, 2022

@kittiu can you see the tests being run? Because I cannot. The same happened to me at my local environments (14.0 and 15.o as well).

@gurneyalex
Copy link
Member

runbot failing: please ignore, this is old story. Runboat seems to be running fine.

@gurneyalex
Copy link
Member

To manually test on runboat:

  • click on the "Details" link next to the runboat/build check
  • Click on the start button on that page if you are not getting on an Odoo page directly, wait ~5s and then click on the "live" link -> this will get you to the Odoo login page.

@Saran440
Copy link
Member

@Rad0van Hello, do you still do this pr?

@Rad0van
Copy link
Contributor Author

Rad0van commented Nov 15, 2022

@Rad0van Hello, do you still do this pr?

Well what more can I do here?

@Saran440
Copy link
Member

@Rad0van runbot is gone. may be you can up-to-date code and push it again.

@Rad0van Rad0van force-pushed the 15.0-mig-base_sequence_option branch from f6ffc28 to 9e08e5a Compare November 16, 2022 11:15
@Rad0van
Copy link
Contributor Author

Rad0van commented Nov 16, 2022

@Rad0van runbot is gone. may be you can up-to-date code and push it again.

Done ;-)

Copy link
Member

@Saran440 Saran440 left a comment

Choose a reason for hiding this comment

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

Functional Test with OCA/hr-expense#149
LGTM 👍

"security/sequence_option_security.xml",
"views/sequence_option_view.xml",
],
"license": "LGPL-3",
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"license": "LGPL-3",
"license": "AGPL-3",

This module should AGPL-3 ?

@@ -0,0 +1,22 @@
# Copyright 2021 Ecosoft Co., Ltd. (http://ecosoft.co.th)
# License LGPL-3.0 or later (https://www.gnu.org/licenses/lgpl.html).
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
# License LGPL-3.0 or later (https://www.gnu.org/licenses/lgpl.html).
# License AGPL-3.0 or later (https://www.gnu.org/licenses/agpl).

All of license should agpl?

Copy link
Contributor Author

@Rad0van Rad0van Nov 17, 2022

Choose a reason for hiding this comment

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

@Saran440 according to https://www.odoo-community.org/resources/faq AGPLv3, LGPLv3 or GPLv3 are possible. Also: I am not in a position to just "change" license of existing module I'm migrating. Only authors can do I believe.

Copy link
Member

Choose a reason for hiding this comment

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

okay, i will change it next PR.
cc: @kittiu

"author": "Ecosoft, Odoo Community Association (OCA)",
"maintainers": ["kittiu"],
"development_status": "Alpha",
"website": "https://github.com/OCA/server-tools",
"category": "Tools",
"depends": ["base"],
"external_dependencies": {"python": ["odoo_test_helper"]},
Copy link
Member

Choose a reason for hiding this comment

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

You don't need to add this here, just add it to test-requirements.txt (or create it if it does not exist 😄 )

Copy link
Contributor Author

@Rad0van Rad0van Nov 25, 2022

Choose a reason for hiding this comment

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

OK, line removed. The test-requirements.txt already contains the odoo_test_helper so didn't do anything there. But it now causes pre-commit to fail. Not really sure I understand what's going on there.

@Saran440
Copy link
Member

@Rad0van Can you update your code again, please?

@patrickt-oforce
Copy link

@pedrobaeza hi, we tested this PR locally, and the pre-commit fail is due to wrong flake8 source url , that pointing to gitlab instead of github, can you make an OCABot rebase?

@pedrobaeza
Copy link
Member

/ocabot rebase

@OCA-git-bot
Copy link
Contributor

Congratulations, PR rebased to 15.0.

@patrickt-oforce
Copy link

@Rad0van i have made a pr versus your branch to fix pre-commit fails

@matteoopenf
Copy link
Contributor

matteoopenf commented Nov 3, 2023

@Rad0van @patrickt-oforce have done a PR from your branch and it's that Rad0van#2 if you merge that the pre-commit works fine

@Rad0van
Copy link
Contributor Author

Rad0van commented Nov 3, 2023

@Rad0van @patrickt-oforce have done a PR from your branch and it's that Rad0van#2 if you merge that the pre-commit works fine

But that removes line from requirements.txt from whole repository. Not really sure that's the way to go...

@matteoopenf
Copy link
Contributor

@Rad0van @patrickt-oforce have done a PR from your branch and it's that Rad0van#2 if you merge that the pre-commit works fine

But that removes line from requirements.txt from whole repository. Not really sure that's the way to go...

we have done a local test and works fine

@patrickt-oforce
Copy link

patrickt-oforce commented Nov 3, 2023

@Rad0van @patrickt-oforce have done a PR from your branch and it's that Rad0van#2 if you merge that the pre-commit works fine

But that removes line from requirements.txt from whole repository. Not really sure that's the way to go...

hi @Rad0van this deletion of requirements is operated by pre-commit, not our initiative; if you want can check results at #2749

@matteoopenf
Copy link
Contributor

@Rad0van the pre-commit now works fine now you should squash the commit and we can ask the merge

@matteoopenf
Copy link
Contributor

@etobella the pre-commit is now fix

@Rad0van Rad0van force-pushed the 15.0-mig-base_sequence_option branch from 64379e5 to 0571ca8 Compare November 3, 2023 14:20
@Rad0van
Copy link
Contributor Author

Rad0van commented Nov 3, 2023

@kittiu @etobella @Saran440 Rad0van with my collegue fix pre-commit and superseed #2749 can you have a look?

Isn't it rather the ocabot rebase fixed the precommit?

@Rad0van Rad0van force-pushed the 15.0-mig-base_sequence_option branch from 964a6c1 to 9c842ec Compare November 3, 2023 14:43
@Rad0van
Copy link
Contributor Author

Rad0van commented Nov 3, 2023

Sorry for the commit/squash mess. Have tried removing that "external_dependencies": {"python": ["odoo_test_helper"]} line from manifest but it keeps breaking despite that library being part of test-requirements.txt:

odoo-test-helper

@matteoopenf
Copy link
Contributor

matteoopenf commented Nov 3, 2023

Sorry for the commit/squash mess. Have tried removing that "external_dependencies": {"python": ["odoo_test_helper"]} line from manifest but it keeps breaking despite that library being part of test-requirements.txt:

odoo-test-helper

have no sense is done by pre-commit.
So, anyway after that is ready to merge @Rad0van what do you think?

@Rad0van
Copy link
Contributor Author

Rad0van commented Nov 3, 2023

Sorry for the commit/squash mess. Have tried removing that "external_dependencies": {"python": ["odoo_test_helper"]} line from manifest but it keeps breaking despite that library being part of test-requirements.txt:

odoo-test-helper

have no sense is done by pre-commit. So, anyway after that is ready to merge @Rad0van what do you think?

Now it passes. I have squashed all the commits so proper order is maintained. I think the rule is 2 approvals and 5 (or 7) days. Someone with rights need to merge it. I do not have those rights...

@OCA-git-bot
Copy link
Contributor

This PR has the approved label and has been created more than 5 days ago. It should therefore be ready to merge by a maintainer (or a PSC member if the concerned addon has no declared maintainer). 🤖

@matteoopenf
Copy link
Contributor

@pedrobaeza I think now is ready to merge.
Do you agree, or need something this pr?

@patrickt-oforce
Copy link

patrickt-oforce commented Nov 3, 2023

@Saran440 i think your PR #2743 and mine can be close; this PR now seem fixed and all check has green mark. About this PR OCA/account-financial-tools#1635 can you try to push an empty commit to restart CI?

@pedrobaeza
Copy link
Member

Merging according reviews.

/ocabot migration base_sequence_option
/ocabot merge nobump

@OCA-git-bot OCA-git-bot added this to the 15.0 milestone Nov 3, 2023
@OCA-git-bot OCA-git-bot mentioned this pull request Nov 3, 2023
54 tasks
@OCA-git-bot
Copy link
Contributor

Hey, thanks for contributing! Proceeding to merge this for you.
Prepared branch 15.0-ocabot-merge-pr-2275-by-pedrobaeza-bump-nobump, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit bdfb103 into OCA:15.0 Nov 3, 2023
9 checks passed
@OCA-git-bot
Copy link
Contributor

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

10 participants