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

Proposal: versionless branches #334

Closed
yajo opened this issue Apr 12, 2018 · 10 comments
Closed

Proposal: versionless branches #334

yajo opened this issue Apr 12, 2018 · 10 comments

Comments

@yajo
Copy link
Member

yajo commented Apr 12, 2018

Milestone (Odoo version)

Upcoming 12.0+

Module(s)

all

Current workflow

Actually we have to maintain separate branches for all Odoo versions. This involves a big work about migrating addons from version n to version n+1, and when patches land in several supported versions, we have to open several PRs with several revisions, etc.

Proposed workflow

IMHO we should start from next Odoo version to avoid affecting current workflow, the upcoming 12.0, and start a new master branch in each repo, that will keep the addons from now on, no matter the Odoo version.

  • From that version, the usual Pythonic workarounds to work with several versions of packages should start to happen. Example:

    try:
        import openerp as odoo  # Stupid example, no more `openerp` needed since Odoo 10, but just so you get the point
    exception ImportError:
        import odoo
  • Migration scripts can either be into folders like migrations/12.0.1.2.0 or like migrations/1.2.0, depending on if the migration affects the Odoo release upgrade, or the addon itself upgrade.

  • Travis jobs could be triggered against all supported versions at once by defining its matrix.

  • Unit tests can make use of skipIf and skipUnless decorators if they belong to specific odoo versions.

  • When adding support for newer versions, those should be considered the default addon version, and old-specific stuff should land in blocks such as:

    if odoo.release.version < "14.0":
        # Specific Odoo 12 stuff
    if odoo.release.version == "12.0":
        ...

IMHO this would be a big leap forward in the maintainability of OCA addons. After all, we maintain addons, not Odoo itself (except for OCB, of course).

@pedrobaeza
Copy link
Member

I don't see this, as each version has very different functional and technical code, so maintaining a module compatible for all versions is almost impossible...

@sbidoul
Copy link
Member

sbidoul commented Apr 12, 2018

That would be nice indeeed in areas where Odoo is relatively stable. There would be some hurdles though.

A few thoughts:

  • we'd need a mechnism to declare supported Odoo versions in the manifest (perhaps Odoo could add it?)
  • what about release-conditional code in JS?
  • what about release-conditional code in XML (views, data)?

Perhaps we could experiment before 12 by enabling the master branch for some repos and see what the impact would be on the various toolchains.

@pedrobaeza
Copy link
Member

@sbidoul making a functional change on the module means to check that change on all the supported versions. That's something simply not feasible.

@sbidoul
Copy link
Member

sbidoul commented Apr 12, 2018

It is feasible for some modules. I do it for mis builder for instance. Many modules in accounting and server-tools would work unchanged from 9 to 11.

Testing across versions is more work for maintainers, that's for sure. But with the current situation we have features or fixes appearing randomly in any version and not being ported. People get bitten by that. So there is a cost too.

So this idea is certainly not trivial to implement and is not applicable everywhere, but I would not dismiss it as impossible right away.

@pedrobaeza
Copy link
Member

OK, I prefer not to do that, but if the whole maintainers of something like mis_builder decides to try this approach... Everything is built around the whole version numbers, so first thing to deal is with that (the MQT check, branch name for testing, etc...).

@yajo
Copy link
Member Author

yajo commented Apr 16, 2018

@sbidoul you nailed it! I think those would be the real problems, since for Python the thing is solved. Even if the addon is extremely different (Python-side), the code could even look like:

if odoo.release.version < "14.0":
    from . import models_13_0
else:
    from . import models

So the real problem is the JS, manifest and data files.

we'd need a mechnism to declare supported Odoo versions in the manifest (perhaps Odoo could add it?)

I'm not sure but I think that you can set the keys you want in the manifest. Another thing would be if they are or not processed by Odoo. We can add a decorator to oca.decorators that helps us decorate the pre_init and pre_update hooks to auto-check if the version is supported when installing or upgrading, and to raise an exception if not.

That would be just the least dirty workaround in case Odoo doesn't add support for such feature, but I think they will like it and add it if a proper PR is sent to master.

what about release-conditional code in XML (views, data)?

I think the <data> element (and its friends) would need to support a couple of new attributes, such as version_min and version_max.

So you can do:

<data>
  <!-- Only for Odoo 12.0 -->
  <data version_min="12.0" version_max="12.0">
    <record>...</record>
  </data>

  <!-- Only for Odoo >= 13.0 -->
  <data version_min="13.0">
    <record>...</record>
  </data>

  <!-- Common stuff -->
  <record>...</record>
</data>

what about release-conditional code in JS?

This case would fall under the same as the above one:

<data>

  <data version_min="14.0">

    <template id="assets_common" inherit_id="web.assets_common">
      <xpath expr=".">
        <link rel="stylesheet"
              href="/module/static/src/css/module_name_14_0.css"/>
        <script type="text/javascript"
                src="/module/static/src/js/module_name_14_0.js"/>
      </xpath>
    </template>
  </data>
</data>

Everything is built around the whole version numbers, so first thing to deal is with that (the MQT check, branch name for testing, etc...)

Some changes would be needed here too indeed. Let me ping @moylop260 who is one of the biggest experts on MQT I think. Do you think this point would be too difficult?


So it seems we are 2 small PRs away from being able to do all of this. IMHO it would be a huge step forward on OCA's maintainability!

Thanks everyone!

yajo added a commit to Tecnativa/odoo that referenced this issue Jun 21, 2018
In order to make possible to implement a different workflow on downstream addon maintenance teams, as explained in OCA/maintainer-tools#334, Odoo needs to support version-specific code.

This can be currently done in Python by checking `odoo.release.version_info` and in JS by using the `/web/webclient/version_info` controller. However, when dealing with data files, we were out of luck.

With this patch:

- A new cached helper is found in JS sessions, that lazy-caches version info once per session, which will make version-specific assertions a pleasure in JS.
- A new attribute `versions` is supported in `<odoo>`, `<openerp>` and `<data>` elements within XML data files, that allows the programmer to specify a version spec that makes that data section be omitted when Odoo version doesn't match it.
- A new `overrides` key is supported in the `__manifest__.py` file, which has the form of a dict, where keys are version specs, and values are dicts that will directly override the addon base manifest.

So, for instance, to declare an addon to be available only until version 13.0 of Odoo, put this in the manifest:

```python
{
  ...
  "installable": False,
  "overrides": {
    ":13": {
      "installable": True,
    }
  }
}
```
@yajo
Copy link
Member Author

yajo commented Jun 30, 2018

After upstream discussion, it's clear that this one is a lost battle. 🙁 Our only chance is to stop supporting official fork and only support OCB, which I feel is another lost battle. I hope some day someone provides a solution that fixes the problems outlined in odoo/odoo#25371 (comment), at least I did all I could.

@yajo yajo closed this as completed Jun 30, 2018
@yajo
Copy link
Member Author

yajo commented Jul 2, 2018

After the initial deception, I have been thinking more on the subject, and we still can do this downstream! 💡

Of course, it will not be as cool as if odoo/odoo#25371 were merged, but still doable and not extremely ugly.

We'd need a new module to serve as base for others. After thinking it for a while, I think we could call it sidekick, or base_helpers or something like that.

The module should add something like this pseudocode:

class Sicekick(models.AbstractModel):
    _name='sidekick'
    @api.model
    def version_load(self, version_spec, data_file):
        ...

This would allow dependent modules to do this:

<data>
 <function model="sidekick" name="version_load">
  <value>11.0:</value>
  <value>/views/res_partner_v11.xml</value>
 </function>
</data>

It would also define the needed JS controllers to make client-side version-specific code easier.

The idea could be good for even providing some useful boilerplate helpers, such as @onwrite(), soft-dependency checks for tests, or a helper to reload data files on tests (as discussed in #213). Indeed it might seem, in the long run, even a better place for the whole oca.decorators package.

The good part of this approach is that it would be 100% in the community's hands, and it would need no hacks - since calling functions in data files is standard and supported since forever - (although, if any hack is needed, it should be in that addon and not in submodules), and it could grow to not only the version-specific code, but other things as well. The bad part is that all data files wouldn't be listed in the manifest; only the cross-version ones.


Now about how this would affect OCA's workflow, because I think that's another matter that we have to address. I see that many people is worried about maintainability of this approach, even calling it "crazy"; however, I find quite crazier current workflow, where source code is maintained separately for different versions.

Implementing this would mean taking some new decisions and embracing new guidelines. Examples:

  1. To avoid the feared spaghetti code, version-specific code should be clearly tagged as such:

    • If possible, always in a separate file suffixed with the version (_v10, _v11...)
    • If not possible, always commented as such with a standard comment like # FIXME Version-specific code
  2. OCA should decide what Odoo versions are supported in master. A good choice could be Odoo officially-supported versions (that's the last 3).

  3. Standard code should target always the latest supported release. Only version-specific code should be suffixed.

  4. Fixes or improvements for newest version only could be allowed. For not-last version, not.

Of course, more things would need to change:

  1. When a version goes out of support, then a specific branch for it is created. For instance, after 4 years from today, when Odoo 15.0 is released and master only covered Odoo 13.0-15.0, a new 12.0 branch would be created. Such branch should be considered stale, and version-specific code for it could start to be removed from master.

  2. If a new version is released with high incompatibility problems (such as v8 was), then it could be considered to create stale branches in that moment, and restore master to support only that newest version.

  3. 3 runbots should run per PR, one per odoo version.

  4. Travis should test same branch in all supported versions simultaneously.

  5. Coverage report should be summed among versions (it can be done according to what @sbidoul told me).

I think a good approach would be to have a testing playground in one or 2 repos. I.e. web & website, where I'm a PSC. Move it to this behavior for 1 year, and then after 1 year, with the real world experience, decide if we prolongate the playground, we move all to this system, or we restore web and website to 1-branch-per-version as they are now.


This implies a big change, both technically and mentally. The benefit is clear to me, but I know haters gonna hate, so I won't personally reopen this. If somebody else feels like this should be done, and wants to help, please express yourself. I won't continue fighting everyone by myself.

@yelizariev
Copy link
Member

This approach can be partially applied, while keeping current structure of repositories.
So, instead of making few pull requests with different code, you can prepare a single PR and let robots make others PRs.
I kind of use it, but instead of code that checks odoo version, I make a comment like

import openerp as odoo
# In Odoo 10.0 use following:
# import odoo

So, I make a PR to 9.0, then it's merged, me or a robot move the code to v10 and then I update the code according to existing comments without thinking about it much.

For python and maybe js it can be easily automated by checking odoo version. For xml this requires updates in odoo framework.

@sebastienbeau
Copy link
Member

@yajo just some feedback from repo where I am PFC (storage, search engine)
I am convinced that this can not be applied to all repo (because some repo are a mix of a lot of different module and migration process is a opportunity to drop obsolete module)
But in some repo (like search-engine, storage, connector, queue_job @guewen ) where all module are really linked together and don't depend of a lot other module, I think it will be great to avoid having one branch per version (We can still have one branch every 2 versions or when there is too much change)

Did you experiment your approach on some repo ?

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

No branches or pull requests

5 participants