[ADD] ocabot migration issue maintainer

[pedrobaeza]

New ocabot command for maintaining migration issues:

/ocabot migration, followed by the module name, performing the following:

• Look for an issue in that repository with the name "Migration to version {version}", where {version} is the name of the target branch.
• Add or edit a line in that issue, linking the module to the pull request (PR) and the author of it.
• TODO: When the PR is merged, the line gets ticked.
• Put the milestone corresponding to the target branch in the PR.

cc @Tecnativa TT21199

[pedrobaeza]

@sbidoul can you take a look and if it looks good for you, deploy it on OCA server for doing real tests?

[pedrobaeza]

I have put the command migration, but if you prefer another one, we can talk about it.

[sbidoul]

@pedrobaeza thanks for working on this.

I'm good with the overall concept. A couple of comments though:

• I would split the setting of the milestone in another PR: that action is not specific for migrations, and can be done on all PRs.
• I'm not 100% comfortable with the use of a command to trigger this action, perhaps it would seem more natural by looking at [MIG] in the subject, or looking for a Migration label? Actually, we could trigger this action on the presence of a Migration label, and have a separate action that sets the Migration label automatically if there is [MIG] in the subject?

[pedrobaeza]

I would split the setting of the milestone in another PR: that action is not specific for migrations, and can be done on all PRs.

Well, I was thinking in being activated manually with this because not all repos follow this milestone way (for example, you with OCA/mis-builder).

I'm not 100% comfortable with the use of a command to trigger this action, perhaps it would seem more natural by looking at [MIG] in the subject, or looking for a Migration label? Actually, we could trigger this action on the presence of a Migration label, and have a separate action that sets the Migration label automatically if there is [MIG] in the subject?

Apart from the reason exposed for checking permissions, there are incorrect migration PRs that shouldn't be pointed until a reasonable quality state is got (I did this for example until commit history is preserved at least). The last reason is that almost nobody respect commit message format, so it's almost impossible to deduce module from this title. Editing the title to conform the specs is worse as:

• It requires permission to do it (only persons with write permissions in repo can do it), so you leave out module maintainers.
• It requires to know a more difficult syntax than the command one.

[pedrobaeza]

@sbidoul what do you think about my replies?

[sbidoul]

@pedrobaeza I understand your argument for making it a command. It makes sense.

Splitting the setting of the milestone in another PR that is independent of this command sounds important. It can simply work by looking at the target branch and setting the milestone or label if one exist with the same name.

Also a couple of unit tests for the inner part of the command will make future maintenance much less stressful :)

[legalsylvain]

Is it still WIP ?

[sbidoul]

My last #97 (comment) is still current. If someone can do this split and add a small test for the inner part that should be good.

[pedrobaeza]

For me the split is not needed and the test is a picky thing. This is working as is.

[sbidoul]

Uh, no, asking for a minimal test is definitely not nitpicking.

Same for the part that sets the milestone: it can be done automatically on all PRs and is totally independent of updating the migration issue. This is basic separation of concerns.

[pedrobaeza]

I don't know how to make tests of this, and simply the setup of all the needed parts is something I can't afford, so all yours.

[pedrobaeza]

@victoralmau can you please catch this one and finish it?

[victoralmau]

Hi @sbidoul i have added some test about these new command.
It's not possible to test it (green) because it's necessary to github access. What idea propose you to solve it?

[legalsylvain]

@victoralmau : could you rebase ? there is a conflict.

@sbidoul : could you answer to the last question of @victoralmau ?

thanks !

[sbidoul]

@victoralmau I see you started using VCR for the tests, and you have factored out the inner code for easier testing. That sounds like the right approach to me. What problem do you have ?

[joao-p-marques]

@pedrobaeza @sbidoul I fixed the tests and I think it should be good now. OTOH, I think the fix from sigmavirus24/github3.py#1048 might be necessary.

[legalsylvain]

Code review.

Thanks for this great feature.

[pedrobaeza]

@sbidoul good for you? The problem is if we can't deploy it without the patch, or maybe deploy that Github patched library?

[joao-p-marques]

@pedrobaeza @sbidoul Turns out the patch was not necessary, I was just creating the wrong test cassete, thinking it was an error caused by an update to the Github API spec.

It should be green now.

[MiquelRForgeFlow]

the yaml files are difficult to review, but LGTM

[joao-p-marques]

the yaml files are difficult to review, but LGTM

Thanks for the review 👍

Well, it is hard to work around that with the current test setup... The other option would be to encode the response body to make it more compact, but at the same time unreadable, so IMHO it is still better this way...

[pedrobaeza]

@sbidoul any news about this? It would be very interesting to have it for the migrations to v15 for saving a lot of maintenance work.

[sbidoul]

I just deployed this. Thanks for your work!