[IMP] add nobump option for 'merge' command. Make bumpversion_mode re…

[legalsylvain]

• Add a fourth key nobump for the option bumpversion_mode of the command merge
• Make bumpversion_mode required. (close Add nobump option to ocabot merge #90)

[sbidoul]

The towncrier commit should not be in this PR.

[sbidoul]

Also don't forget newsfragments/90.feature

[legalsylvain]

Hi @sbidoul thanks for your review.
In fact, the PR is still WIP. travis fails for the time being, no fragment file, etc. I just added a label, and I'll ping you when the PR is ready.

[legalsylvain]

@sbidoul : Now ready for review. Thanks !

[legalsylvain]

Also don't forget newsfragments/90.feature

Side question. i was thinking to add a 110.feature (the current PR) file, but you asked a 90.feature (the original Issue) file. I so followed your advice ;-)
But is there some guideline in OCA documentation that says how to use towncrier ? Good practice, etc.
Not found in the website nor in odoo-community doc repository.

[legalsylvain]

Hi @sbidoul. Thanks for your help.

-> I revert the refactor of the parse_options() of the class BotCommandMerge. It was an attempt to handle a more complex parsing, when there will be more options available. But I let this work for people that will implement the \ocabot merge minor squash feature. ;-)

-> I reverted also the changes that makes the branch name pattern changed. bump-no versus bump-nobump to avoid to break current jobs, when upgrading.

-> I just kept the possible values to ["minor", "major", "patch", "nobump"] and I didn't wrote ["minor", "major", "patch", None] list as you proposed. I see it less logic, for a required field. Hope it will not be a blocking point.

I let the fixup commit, to make more easy to read the diff. Ping me if all is OK, and if you want me to squach before merging.

[sbidoul]

One last question: what happen if people forget the new required "nobump" options?
If it fails silently, user will not understand why their command does nothing.
So we probably want to report a GitHub comment to avoid too much support headaches with the change.

[sbidoul]

Nitpick I would have done this to avoid changing the rest of the code. No big deal though.

Suggested change

	self.bumpversion_mode = options[0]
	if options[0] != 'nobump':
	self.bumpversion_mode = options[0]

[legalsylvain]

Yes I get it. You're right, diff could be lighter.
The problem I see with such implementation is for developpers. IMO,

• if option is optional, the developer expect that bumpversion_mode is in ["major", "minor", "patch", None]
• if option is required, the developer expects that bumpversion_mode is in ["major", "minor", "patch", "nobump"]

I so implemented that point with such design. If it's a blocking point, I revert, no problem.

[legalsylvain]

One last question: what happen if people forget the new required "nobump" options?
If it fails silently, user will not understand why their command does nothing.
So we probably want to report a GitHub comment to avoid too much support headaches with the change.

Nice catch !

In fact, it's not a problem of that PR, it's a general limitation of this bot. For the time being, if we write ocabot make-me-a-coffee or ocabot merge it will fail silently. But, well, with the change of the current feature, setting an option required, it will generate a lot of mails and questions.

I so tried that changes, I set in a PR against my PR :

• here is the code : Add nobump version add usage message legalsylvain/oca-github-bot#1
• here is the result : Update __manifest__.py grap-org-test-bot/github-ocabot-test#10

I propose you to include this feature in that current PR. It breaks "the one feature per fragment" law, but I think that it should merge together, and the second feature depends on the first one.

Do you like it ? Let me know !

[sbidoul]

@legalsylvain nice error messages, it's a great improvement, thanks. I made a couple of comments on the implementation over there.

[legalsylvain]

@legalsylvain nice error messages, it's a great improvement, thanks. I made a couple of comments on the implementation over there.

Hi @sbidoul. I took into account your remarks :

• here is the code : Add nobump version add usage message legalsylvain/oca-github-bot#1
• here is the result : Update __manifest__.py grap-org-test-bot/github-ocabot-test#11

kind regards.

[legalsylvain]

@sbidoul. I took into account your remarks and squashed fixup! commits. could you take a look, and merge if all is OK ? Thanks !

[sbidoul]

@legalsylvain thanks for your work on this.

One last thing: could you add the error comment in a task named, e.g. add_pr_comment. Similar to what is done in on_pr_open_mention_maintainer. The reason we want to do that is that the tasks have a built-in retry mechanism in case we hit the GitHub rate limit. Another reason is that the webhooks are based on the gidgethub library which is using asyncio, while the rest is based on the github3 library which uses synchronous IOs.

[legalsylvain]

Hi @sbidoul.
thanks again ! this should be OK, now.

Test : grap-org-test-bot/github-ocabot-test#12

[legalsylvain]

Hi @sbidoul. Do you have some time to finish the review and merge this PR ?
Thanks.

[sbidoul]

LGTM. Thanks for this @legalsylvain!

Now we'll need to communicate to the contributors before deploying.

[legalsylvain]

Thanks !
I'll send an email to the OCA members.

[legalsylvain]

@sbidoul : Done here : https://odoo-community.org/groups/contributors-15/contributors-160206?mode=thread

Please answer to that mail, when you update in production the ocabot.

Thanks for your time.