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

Support repos with conflicting addons #36

Open
rousseldenis opened this issue Dec 15, 2021 · 27 comments
Open

Support repos with conflicting addons #36

rousseldenis opened this issue Dec 15, 2021 · 27 comments
Labels
enhancement New feature or request

Comments

@rousseldenis
Copy link

https://runboat.odoo-community.org/api/v1/builds/b552cf370-23e2-4ce6-b081-f2a76e741cc9/init-log

All runboat builds are failing on 14.0 branch in sale-workflow.

@sbidoul @lmignon

@rousseldenis
Copy link
Author

image

@sbidoul
Copy link
Owner

sbidoul commented Dec 15, 2021

This is due to modules having the excludes key in the manifest.

runbot did handle that by parsing the travis configuration (via travis2docker) and retaining only the configuration for the first job, therefore takiing into account the INCLUDE/EXCLUDE variables.

Parsing the CI config in runboat is definitely not an option.

I currently think about two approaches to handle this:

  1. trying to automatically determine a subset of compatible modules and installing them in the database, leaving it up to the user to uninstall some and install others if they want to test another subset
  2. generating multiple builds for single commits

Approach 1 sounds hard to implement (well not that hard, but requires some graph analysis wizardry that should go into manifestoo)

Approach 2 first requires supporting multiple builds for a single commit. There is an embryo of that, since the build settings is a list.
Then we need to communicate to runboat the addons groups to put in each build. Which hints at some kind of .runboat.yaml at the repo root. Which opens the door to... versatility but also complexity.

There might be other approaches. Not sure what's best.

@rousseldenis
Copy link
Author

This is due to modules having the excludes key in the manifest.

runbot did handle that by parsing the travis configuration (via travis2docker) and retaining only the configuration for the first job, therefore takiing into account the INCLUDE/EXCLUDE variables.

Parsing the CI config in runboat is definitely not an option.

I currently think about two approaches to handle this:

  1. trying to automatically determine a subset of compatible modules and installing them in the database, leaving it up to the user to uninstall some and install others if they want to test another subset
  2. generating multiple builds for single commits

Approach 1 sounds hard to implement (well not that hard, but requires some graph analysis wizardry that should go into manifestoo)

Approach 2 first requires supporting multiple builds for a single commit. There is an embryo of that, since the build settings is a list. Then we need to communicate to runboat the addons groups to put in each build. Which hints at some kind of .runboat.yaml at the repo root. Which opens the door to... versatility but also complexity.

There might be other approaches. Not sure what's best.

What was the process in runbot ?

Shouldn't we in that case see case by case if those kind of modules should be in separate repos (but there can be limitations)?

@sbidoul
Copy link
Owner

sbidoul commented Dec 15, 2021

What was the process in runbot ?

Have you read what I wrote ? runbot did handle that by...

Shouldn't we in that case see case by case if those kind of modules should be in separate repos (but there can be limitations)?

That is another option, but incompatible modules in the same repo is moderately frequent in OCA, even if they are not explicitly declared as such in their manifests.

@sbidoul sbidoul added the enhancement New feature or request label Dec 15, 2021
@rousseldenis
Copy link
Author

Have you read what I wrote ? runbot did handle that by...

Missed that part 😶‍🌫️

@petrus-v
Copy link

Haven't follow that much CI OCA improvement last few years, @sbidoul does the goal of runboat to replace runbot and travis ? I've the feeling that things are tested twice !

A bad option could be to ignore some modules (at least in the mean time)... but this will open the doors of bike-shading !

Matrix build defined in config file seems a nice improvement that could be used for other purpose ! Wondering if there are an easy things to do in the mean time to leave Green PR in the mean time in such case ?

@rousseldenis
Copy link
Author

@sbidoul does the goal of runboat to replace runbot and travis ?

The goal was to replace only runbot as it was quite difficult to maintain and was based on non-standard flows.

Travis will remain obviously for tests

@sbidoul
Copy link
Owner

sbidoul commented Dec 15, 2021

does the goal of runboat to replace runbot and travis ? I've the feeling that things are tested twice !

Runboat is not for running tests. Only installation for manual testing.

Travis is being replaced by GitHub actions for running tests and linters.

@petrus-v
Copy link

Oh sorry haven't takes time to deep into details this happens while installing modules 🤔

And my proposal is some how what runbot is doing today with explicit configuration which you would like to avoid as far I understand !

Thanks for details and all massive works, I've to investigate more to tell something clever on the topic sorry for the nose !

@sbidoul sbidoul changed the title Two incompatible modules make build failed Support repos with conflicting addons Dec 22, 2021
@sbidoul
Copy link
Owner

sbidoul commented Dec 22, 2021

@petrus-v no worries! I hope I did not come across as dismissive, it certainly was not my intent.

@sbidoul
Copy link
Owner

sbidoul commented Dec 22, 2021

If we decide to implement a .runboat.yaml with an addons matrix, the question of naming the matrix elements will come up. Currently in the oca-addons-repo-template, the copier question about rebel module groups does not ask to name the groups.

Also the matrix would need to be combined with build settings names to create build names.

The names will need to be short enough to reasonably fit in the build cards in the UI.

@petrus-v
Copy link

@petrus-v no worries! I hope I did not come across as dismissive, it certainly was not my intent.

I don't feel you were dismissive :). You are very patient and I'm amazed with all you're doing.

@rousseldenis
Copy link
Author

If we decide to implement a .runboat.yaml with an addons matrix, the question of naming the matrix elements will come up. Currently in the oca-addons-repo-template, the copier question about rebel module groups does not ask to name the groups.

Also the matrix would need to be combined with build settings names to create build names.

The names will need to be short enough to reasonably fit in the build cards in the UI.

In order to replace definetly runbot by runboat, those questions need to be explored as the current implementation is failing when conflict occurs (even if splitting modules in specific repositories could be a solution, but not universal and time consuming).

@sbidoul
Copy link
Owner

sbidoul commented Dec 22, 2021

@rousseldenis or conflict modules can be put in different repos. Besides sale-workflows, are there other places where runboat fails due to this ?

@rousseldenis
Copy link
Author

@rousseldenis or conflict modules can be put in different repos. Besides sale-workflows, are there other places where runboat fails du to this ?

I'm not aware of but don't have sight everywhere 😅

@sbidoul
Copy link
Owner

sbidoul commented Dec 22, 2021

Ok, let us see. As as long as it affects only one repo, my motivation to do this complex change is fairly low.

@petrus-v
Copy link

I believe that there are some repos like account-analytic where there are exclude rules in travis (not declared as rebel module in copier) for unit-test there is no issue while installing both modules on the instance but I expect using runboat odoo will failled at some point having modules not compatible.

Not sure who is using runboat and how it is a matter, I guess few people will lose time trying to understand some modules are not compatible ?

my 2cts !

@SirTakobi
Copy link

SirTakobi commented Apr 22, 2022

Another case of incompatible modules: OCA/account-invoicing#1143
No more conflicting

@bealdav
Copy link

bealdav commented Apr 7, 2023

OK thanks for this analyze.

What is the situation now ?

  • travis have been fully replaced by github actions
  • github actions are now nicely updated by copier config
  • copier config contains rebel_module_groups to impact test.yml

There is no duplicate information; nice ! @sbidoul

Could runboat use rebel_module_groups config to install the whole repo except rebel ones without any other setting file ?

If yes, first Stéphane's approach is possible I suppose.

Did I forgot something ?

cc @sbidoul @rousseldenis

@sbidoul
Copy link
Owner

sbidoul commented Apr 7, 2023

I want to avoid coupling runboat with unrelated configuration files such as .copier-answers.yml or .github workflows.

A .runboat.yml could provide environment variables that are passed to the containers. For OCA, an EXCLUDE variable could be sufficient as the oca-ci image would then not install these addons in the database, but still make them available for manual installation (IIRC).

The OCA copier template could then populate that with the list of all rebel modules.

I'd welcome a PR exploring that approach.

@bealdav
Copy link

bealdav commented Apr 7, 2023

Thanks @sbidoul.
I don't know if you can define the expected result. But if so, I could check if I can make it works!?

@sbidoul
Copy link
Owner

sbidoul commented Apr 7, 2023

One thing that maybe easy to implement and generic at the same time is to let runboat-initialize.sh and runboat-start.sh source $ADDONS_DIR/.env.runboat if it exists.

And then update oca/oca-addons-repo-template to generate .env.runboat with EXCLUDE=rebel modules if needed.

@yajo
Copy link

yajo commented May 10, 2023

I've hit this bug today in OCA/project#1101.

Keeping in mind the proposal at #94, we could tell runboat to always generate "base" and "all" databases, but we could tell the repo template that, if there are rebel modules, somehow it should tell runboat to skip the "all" database.

@bealdav
Copy link

bealdav commented Sep 18, 2023

Finally it seems .env.runboat is a convenient way to solve the problem but I'm not sure how to implement it as described by @sbidoul.
As said by @yajo, if there are rebel modules we could skip all database.

Moreover it could be appreciate to have en empty db to only install the use case that you want without a bloated UI.

I don't dare to propose to only have an empty db because it's an radical change but it could have advantages :

  • better for the planet with shorter runboat up
  • better for OCA finance with less ressources used
  • you could test with UI with only what you want.
  • changes in runboat code should be very easy, no need to populate and check .env.runboat, only base to install

Then to summarize

1/ add additional runboat install with base module and some code to add to manage it
2/ replace all by base

Any of these choices are convenient for me, but I think we should move because I'm blocked here OCA/stock-logistics-workflow#1271

Thanks a lot

@sbidoul
Copy link
Owner

sbidoul commented Sep 18, 2023

I won't have much time to work on this in the coming weeks, but if anyone wants to contribute I'll be happy to help at the OCA Days sprint in Liège. At this point I have a preference for #94 and its associated draft PR rather than .env.runboat (although new requirements may still change my mind).

@bealdav
Copy link

bealdav commented Sep 18, 2023

Ok, nice I haven't seen #95.

Thanks for that.

As I understand your code if we merge it in this state, we always have db initialized base and all, and it should be sufficient to make PRs with rebel modules green and mergeable . But i understand you want to go further.
Unfortunately I can't be in Liège this year.

Thanks a lot

@sbidoul
Copy link
Owner

sbidoul commented Sep 18, 2023

and it should be sufficient to make PRs with rebel modules green and mergeable

Note that even today, a red runboat is not blocking merge. It just means the PR is not testable on runboat.

Unfortunately I can't be in Liège this year.

Oh, too bad.

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

No branches or pull requests

6 participants