[12.0][MIG] auth_ldaps

[alexey-pelykh]

No description provided.

[oca-clabot]

Hey @alexey-pelykh, thank you for your Pull Request.

It looks like some users haven't signed our Contributor License Agreement, yet.
You can read and sign our full Contributor License Agreement here: http://odoo-community.org/page/cla
Here is a list of the users:

• @alexey-pelykh (login unknown in OCA database)

Appreciation of efforts,
OCA CLAbot

[pedrobaeza]

Thanks for the contribution!

runbot warnings:

2018-10-16 06:26:51,079 150 WARNING openerp_test odoo.models: The model res.company.ldap has no _description

Have you signed the CLA?

[alexey-pelykh]

re: CLA - yes, just email one.
re: warnings - not sure what to add there as original model does not specify any

[pedrobaeza]

Well, you can add the description on your inherited model.

[alexey-pelykh]

I'm not sure that adding a description in inherited model is a good approach, since eventually original model may get one. Is there a strict requirement of having that description in inherited model?

[pedrobaeza]

This is for avoiding the warning on the runbot, so the CIs are green, so it's mandatory on that way. Now you have also Travis red.

[alexey-pelykh]

Yeah, lint :) Fixed both issues, hopefully would work now

[alexey-pelykh]

@pedrobaeza I've added the description yet it still fails for some reason, a hint would be much appreciated

[alexey-pelykh]

Strictly speaking, I don't see a _description for this case here, please advise

[pedrobaeza]

Hi @alexey-pelykh, it turns out that the trick of putting the description in the inherited model doesn't server for avoiding the warning, so here the only solution is to ignore the warning and tell Odoo to fix it in the core (or make a PR fixing it).

[alexey-pelykh]

Got it. Are there any other changes needed for this PR to be accepted?

[pedrobaeza]

Well, now this one should be technically and functionally reviewed (mine was only "kick-off review"). Check review guidelines in https://github.com/OCA/maintainer-tools/blob/master/CONTRIBUTING.md#review

If you want to speed up your PR, you can review other PRs and ask in exchange that they review yours.

[alexey-pelykh]

Thanks for the hint! Just to make sure I've understood you correctly, now the ball is on OCA side and I should just wait?

[pedrobaeza]

No, that's not what I mean. It's not on the OCA side, but on contributors side, and also on your side. If you want your PR to be reviewed, you should do the same on other PRs. That's how collaborative efforts work!

[sebalix]

There was an issue regarding this import: #37
Can you fix it by protecting the import?

try:
    import ldap
except (ImportError) as err:
    _logger.debug(err)

https://github.com/OCA/maintainer-tools/blob/master/CONTRIBUTING.md#importerror

[sebalix]

And put it in a separate commit (no squash) to backport it easily on 11.0.

[alexey-pelykh]

Even if it's a mandatory dependency of the "parent" module?

[alexey-pelykh]

Done

[lmignon]

Thank you @alexey-pelykh fot this work.
A little comment. (Code review only)

[lmignon]

LGTM Code review only

[alexey-pelykh]

Next step on my side is to wait, right?

[alexey-pelykh]

Runbot warnings are going to be present until odoo/odoo#28047 is merged

[alexey-pelykh]

@pedrobaeza all green and 5 days seems to have passed, what should I do next?

[pedrobaeza]

Please squash your 3 migration commits in one following https://github.com/OCA/maintainer-tools/wiki/Merge-commits-in-pull-requests and I'll merge.

[alexey-pelykh]

But there was a specific request to make those 2 changes separately by @lmignon and @sebalix to backport those to 10.0 and 11.0

[pedrobaeza]

@lmignon and I have different notion of commits. I'm more commit saving as they don't usually express so much being split (although there are of course exceptions), but he seems to collect commits 😋

I think you should at least squash last 2 as they are about the same thing and will be cherry-picked together, but I let you the final word.

[alexey-pelykh]

Sounds reasonable! Done

[lmignon]

@pedrobaeza In this specific case merging the two last commit makes senss

[alexey-pelykh]

@pedrobaeza do these modules get released to pypi.org?

[pedrobaeza]

It should through our bots, but I'm not too aware of the process. I'm seeing the setup directory in https://github.com/OCA/server-auth/tree/11.0/setup/auth_ldaps, but I can't say if everything is OK. @lmignon or @sbidoul can say about that.

[sbidoul]

@alexey-pelykh It is on pypi: https://pypi.org/project/odoo12-addon-auth-ldaps/#history

[alexey-pelykh]

Thanks! For some reason, search did not bring it up

[alexey-pelykh]

And to https://apps.odoo.com/apps as well?

[sbidoul]

Ah that's another story. Someone has to manually add the 200 or so 12.0 branches on apps.odoo.com... I don't think that has been done yet.

[oca-clabot]

Hey @alexey-pelykh,
We acknowledge that the following users have signed our Contributor License Agreement:

• @alexey-pelykh

Appreciation of efforts,
OCA CLAbot