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

Test only supported Odoo versions #189

Merged
merged 1 commit into from
Mar 28, 2023

Conversation

SirTakobi
Copy link
Contributor

@SirTakobi SirTakobi commented Mar 23, 2023

Implementing what suggested in

The best bet is to modify the OCA CI to only use OCB. At the end, there won't be any counterpart, as there are no more Odoo updates, so if right now is green, the only chance to break it is on OCB. And we will gain less energy consumption in the builds.

Originally posted by @pedrobaeza in odoo/odoo#115782 (comment)

The issue fixed in the linked PR is blocking many good PRs in 12.0 out there.

Here I allow to choose if tests should be executed in Odoo/OCB.

I don't know how to test this locally so I just wrote what seemed appropriate, any feedback is appreciated.
I tried removing the old .copier-answers from a repository and copier /path/to/oca-addons-repo-template . but it isn't using the edited template that I have locally.

@SirTakobi SirTakobi marked this pull request as ready for review March 23, 2023 15:04
Copy link
Contributor

@legalsylvain legalsylvain left a comment

Choose a reason for hiding this comment

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

The idea look great 👍 ! I'm always OK to reduce the impact of the CI when tests are unnecessary.

About the selection of the version, 13.0 is not a good choice for the time being (and maybe 12.0)
See :

as a safe choice, I would therefore propose the following rule:
We can remove the odoo "X.0" branch from the OCA CI, and use only OCB if there is no commit on the "X.0" branch for one year.

So for the time being : 10.0 and 11.0. (and remove 12.0 in October 2023, if no new commits are pushed).

What do you think ?

@SirTakobi SirTakobi force-pushed the unsupported_odoo_versions branch from 047327e to 53d481f Compare March 23, 2023 15:39
@SirTakobi
Copy link
Contributor Author

Thanks for having a look!

The idea look great 👍 ! I'm always OK to reduce the impact of the CI when tests are unnecessary.

About the selection of the version, 13.0 is not a good choice for the time being (and maybe 12.0) See :

as a safe choice, I would therefore propose the following rule: We can remove the odoo "X.0" branch from the OCA CI, and use only OCB if there is no commit on the "X.0" branch for one year.

So for the time being : 10.0 and 11.0. (and remove 12.0 in October 2023, if no new commits are pushed).

What do you think ?

I have restored 13.0 because:

  • looks like it is still updated: other than the last commit, there is roughly one commit every ~10 days
  • I am actually not aware of any issue in that version.

I would keep excluding 12.0 because:

  • right now it is failing;
  • being the last update 6 months ago, I have some doubts it will ever be fixed;
  • last but not least, I am still using it 😄, so in order to contribute to OCA I need the CI to be working.

@SirTakobi SirTakobi force-pushed the unsupported_odoo_versions branch from 53d481f to f8d770c Compare March 23, 2023 15:53
@SirTakobi SirTakobi requested a review from legalsylvain March 23, 2023 15:55
Copy link
Contributor

@legalsylvain legalsylvain left a comment

Choose a reason for hiding this comment

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

for me makes sense. But I'm not expert of that repo, so other review are welcome.

thanks for the changes !

Copy link
Member

@yajo yajo 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 can you please confirm that the makepot change should behave as expected?

@sbidoul
Copy link
Member

sbidoul commented Mar 24, 2023

I think it would be more flexible to add a copier question to enable odoo and/or ocb tests.
This way the odoo test can be disabled by default on older branches but people can still enable them if they want to.

@SirTakobi
Copy link
Contributor Author

I think it would be more flexible to add a copier question to enable odoo and/or ocb tests. This way the odoo test can be disabled by default on older branches but people can still enable them if they want to.

Good idea, that would be more flexible for sure, I should look up how to do that.
But other than possible technical problems, how should the question behave?

  • Choose Odoo/OCB for all versions?
    Maybe is too extreme
  • Or choose Odoo/OCB for each version?
    Dynamic number of questions: is that even possible?
  • Or something in between?
    Maybe we ask only for specific versions, like the ones I am already excluding here

@sbidoul
Copy link
Member

sbidoul commented Mar 27, 2023

I think we can have a question such as "Which Odoo flavor to test?" with 3 possible answers: odoo/OCB/both.

@SirTakobi
Copy link
Contributor Author

I think we can have a question such as "Which Odoo flavor to test?" with 3 possible answers: odoo/OCB/both.

Right, I forgot that each template is used only for one version at a time 😅 ok then I'll look up how to use the questions to do this.

@SirTakobi SirTakobi marked this pull request as draft March 27, 2023 08:13
@bealdav
Copy link
Member

bealdav commented Mar 27, 2023

Good initiative here.

@SirTakobi SirTakobi force-pushed the unsupported_odoo_versions branch from f8d770c to a54d5da Compare March 27, 2023 11:54
@SirTakobi SirTakobi marked this pull request as ready for review March 27, 2023 12:09
@SirTakobi SirTakobi requested review from yajo and legalsylvain and removed request for yajo and legalsylvain March 27, 2023 12:09
@SirTakobi
Copy link
Contributor Author

🤔 sorry for the multiple review requests, apparently I can't re-request review from both @yajo and @legalsylvain.

Anyway, I re-requested the reviews because something changed: now the user can choose what to enable, let me know what you think 😄
cc @sbidoul

@sbidoul
Copy link
Member

sbidoul commented Mar 27, 2023

@SirTakobi does it enable makepot in case OCB only is selected?

@SirTakobi
Copy link
Contributor Author

@SirTakobi does it enable makepot in case OCB only is selected?

Yes because makepot is added to the last configuration, regardless of what was selected

@SirTakobi SirTakobi force-pushed the unsupported_odoo_versions branch from 0934527 to aeb59b9 Compare March 27, 2023 13:32
@sbidoul
Copy link
Member

sbidoul commented Mar 27, 2023

@SirTakobi we must ensure to not run makepot twice. So it should appear in OCB jobs only it the flavor is OCB.

@SirTakobi
Copy link
Contributor Author

@SirTakobi we must ensure to not run makepot twice. So it should appear in OCB jobs only it the flavor is OCB.

Looking at the code changes, I think it will only be run once; of the three possible choices:

  • "Odoo": makepot is run in "Odoo"
  • "OCB": makepot is run in "OCB"
  • "Both": makepot is run in "OCB"

Have you noticed some case in the code that I'm missing?

@sbidoul
Copy link
Member

sbidoul commented Mar 27, 2023

Ah, yes, sorry I misunderstood. 👍

1 similar comment
@sbidoul
Copy link
Member

sbidoul commented Mar 27, 2023

Ah, yes, sorry I misunderstood. 👍

@yajo yajo merged commit ff96cae into OCA:master Mar 28, 2023
@yajo
Copy link
Member

yajo commented Mar 28, 2023

Thanks!

Would you please apply this into one or two repos before we do a release? So we can see it works in the real world. Update to latest unreleased change with:

copier -r HEAD update

@SirTakobi
Copy link
Contributor Author

Thanks!

Would you please apply this into one or two repos before we do a release? So we can see it works in the real world. Update to latest unreleased change with:

copier -r HEAD update

Done in OCA/l10n-italy#3241

@@ -76,6 +76,15 @@ ci:
- GitHub
help: Which CI system to use ?

odoo_test_flavor:
type: str
default: Both
Copy link
Member

Choose a reason for hiding this comment

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

Do we want to set the default value to OCB for Odoo < 13 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes that might be useful

but honestly I don't know how to do that, is that even possible? I mean, this is not a jinja template

Copy link
Member

Choose a reason for hiding this comment

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

We already have default: "https://github.com/{{ org_slug }}/{{ repo_slug }}" so we probably can put any python expression in there?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, you can do that.

@SirTakobi
Copy link
Contributor Author

SirTakobi commented Mar 28, 2023 via email

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.

6 participants