OCA / interface-github

Tools to interact with github from Odoo
GNU Affero General Public License v3.0
46 stars 77 forks source link

[FIX]do not delete module version if already exists and just update #30

Closed bizzappdev closed 5 years ago

bizzappdev commented 6 years ago

If Odoo module version already exists then instead of updating that version it is deleting and creating a new one.

Expected behaviour after this PR, It will not delete the existing version.

Updating the existing version functionality is already there

bizzappdev commented 6 years ago

https://github.com/OCA/apps-store/issues/31 will be solved after this PR merged.

bizzappdev commented 6 years ago

@RoelAdriaans @oscarolar @StephanRozendaal Can anyone of you please review this.

pedrobaeza commented 6 years ago

Why Travis is red?

bizzappdev commented 6 years ago

@pedrobaeza https://github.com/OCA/interface-github/blob/11.0/github_connector/models/github.py#L75 this is the normal python Class and Travis is expecting a Super call for create method https://github.com/OCA/interface-github/blob/11.0/github_connector/models/github.py#L156

changes are Not from my commit.

StephanRozendaal commented 6 years ago

Looks like the flake8 test failed, in the log I see:

github_connector_odoo/models/github_repository_branch.py:88:13: F841 local variable 'module_versions' is assigned to but never used

pedrobaeza commented 6 years ago

Yes, you should remove previous line too. But I wonder why it was there initially, as this seems to be put on purpose.

@legalsylvain @cubells any idea?

bizzappdev commented 6 years ago

Not needed code removed. thanks @StephanRozendaal @pedrobaeza No idea why it was there before. might be the original author can answer

bizzappdev commented 6 years ago

@legalsylvain okay clear, You mean to say at the end of the cron job we should call the module and module version verification method. I will work on that tomorrow

elicoidal commented 6 years ago

I think we should replace this code by another, to test if the module still exist, and if not, delete obsolete module version.

Makes sense to me too

legalsylvain commented 6 years ago

Not exactly. I mean : After first analysis : a repository contains 5 module.version.

when we analyse again, if a module.version disappeared from the repository we should drop it. (and so the appstore should drop the related products also)

so kind of algorithm like :

# at the beginning of the analyse of a repository, save current_module_version_ids

# during the analyse save all the currently analyse module version into a analysed_module_version_ids

# at the end, drop all module versions that are in current_module_version_ids - analysed_module_version_ids

(something like that)

elicoidal commented 6 years ago

should drop the related products also

you mean inactivate. When it reappears we just reactivate the product.

@hbrunn has an interesting idea that we could develop later to create products in Alpha maturity level for open PR with green travis. We should be able to activate/inactivate a version after discovery IMO

bizzappdev commented 6 years ago

Not exactly. I mean : After first analysis : a repository contains 5 module.version.

when we analyse again, if a module.version disappeared from the repository we should drop it. (and so the appstore should drop the related products also)

so kind of algorithm like :

# at the beginning of the analyse of a repository, save current_module_version_ids

# during the analyse save all the currently analyse module version into an analysed_module_version_ids

# at the end, drop all module versions that are in current_module_version_ids - analysed_module_version_ids

(something like that)

this seems little tricky as we do process always all the branches. we just project the branches which have state to_analyze. IMO we should have another special cron job to check the drop odoo module version. which we can enhance in the app-store module to archive the product.

elicoidal commented 6 years ago

IMO we should have another special cron job to check the drop odoo module version. which we can enhance in the app-store module to archive the product.

Looks like a good idea to one specific cron job to have one pass on all module and change the active field indeed

bizzappdev commented 6 years ago

as per that, I have added a new cron job for cleaning up the Odoo module version. @legalsylvain Can you please review

bizzappdev commented 6 years ago

@elicoidal yes the flow is exactly what you say. in interface-github only the code is for deleting the module version while on apps-store module we have code for achieving the product.

bizzappdev commented 6 years ago

@legalsylvain changes are done as per your suggestion. and module_paths is Now supported.

legalsylvain commented 6 years ago

and module_paths is not supported.

I don't understand that point. I'm running this module in local instance, and I analyse correctly modules, downloading the code of https://github.com/odoo/odoo and the analysis of the modules of Odoo core works fine.

bizzappdev commented 6 years ago

and module_paths is not supported.

I don't understand that point. I'm running this module in local instance, and I analyse correctly modules, downloading the code of https://github.com/odoo/odoo and the analysis of the modules of Odoo core works fine.

Sorry for the previous update. "Now" is autocorrected in mind by "Not". and that has changed the complete meaning of the Sentence.

legalsylvain commented 6 years ago

OK. ;-). note : I don't see the changes, did you commited ?

bizzappdev commented 6 years ago

indeed push was not done.

bizzappdev commented 6 years ago

@StephanRozendaal can you please review again because few things are changed after your approval.

elicoidal commented 5 years ago

@StephanRozendaal are we good to go?

elicoidal commented 5 years ago

thanks @StephanRozendaal merging