Skip to content

Conversation

@marielejeune
Copy link

No description provided.

@sbidoul
Copy link
Member

sbidoul commented Mar 15, 2023

/ocabot migration base_automation

@OCA-git-bot OCA-git-bot added this to the 16.0 milestone Mar 15, 2023
@marielejeune marielejeune force-pushed the 16.0-base_automation-mle branch from 8da140a to 4b6e08d Compare March 27, 2023 14:00
@marielejeune marielejeune force-pushed the 16.0-base_automation-mle branch from 4b6e08d to 4fef924 Compare March 27, 2023 14:40
@marielejeune marielejeune changed the title [16.0][MIG] base_automation: Nothing to do [16.0][OU-ADD] base_automation: Nothing to do Apr 6, 2023
Copy link

@remytms remytms left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

Copy link
Contributor

@legalsylvain legalsylvain left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/ocabot merge patch

@legalsylvain
Copy link
Contributor

/ocabot merge patch

@OCA-git-bot
Copy link
Contributor

Hey, thanks for contributing! Proceeding to merge this for you.
Prepared branch 16.0-ocabot-merge-pr-3779-by-legalsylvain-bump-patch, awaiting test results.

OCA-git-bot added a commit that referenced this pull request Jul 1, 2023
Signed-off-by legalsylvain
@OCA-git-bot
Copy link
Contributor

What a great day to merge this nice PR. Let's do it!
Prepared branch 16.0-ocabot-merge-pr-3779-by-legalsylvain-bump-patch, awaiting test results.

OCA-git-bot added a commit that referenced this pull request Jul 1, 2023
Signed-off-by legalsylvain
@OCA-git-bot
Copy link
Contributor

This PR has the approved label and has been created more than 5 days ago. It should therefore be ready to merge by a maintainer (or a PSC member if the concerned addon has no declared maintainer). 🤖

@OCA-git-bot
Copy link
Contributor

@legalsylvain your merge command was aborted due to failed check(s), which you can inspect on this commit of 16.0-ocabot-merge-pr-3779-by-legalsylvain-bump-patch.

After fixing the problem, you can re-issue a merge command. Please refrain from merging manually as it will most probably make the target branch red.

@legalsylvain
Copy link
Contributor

/ocabot merge patch

@OCA-git-bot
Copy link
Contributor

Hey, thanks for contributing! Proceeding to merge this for you.
Prepared branch 16.0-ocabot-merge-pr-3779-by-legalsylvain-bump-patch, awaiting test results.

OCA-git-bot added a commit that referenced this pull request Jul 1, 2023
Signed-off-by legalsylvain
@OCA-git-bot
Copy link
Contributor

@legalsylvain your merge command was aborted due to failed check(s), which you can inspect on this commit of 16.0-ocabot-merge-pr-3779-by-legalsylvain-bump-patch.

After fixing the problem, you can re-issue a merge command. Please refrain from merging manually as it will most probably make the target branch red.

@pedrobaeza
Copy link
Member

No need to launch ocabot, and less with version bumping, as there's no sense in that. Merging directly.

@pedrobaeza pedrobaeza merged commit b123f15 into OCA:16.0 Jul 2, 2023
@sbidoul sbidoul deleted the 16.0-base_automation-mle branch July 2, 2023 10:22
@legalsylvain
Copy link
Contributor

No need to launch ocabot

We've already talked about it. Yes, I think it's important. You can disagree, but please don't say the same thing over and over again, without argument.

@pedrobaeza
Copy link
Member

I don't remember any counter argument on your part.

@legalsylvain
Copy link
Contributor

I don't remember any counter argument on your part.

I can't find the PR where we both talked about this subject.
If you'd like, we can open a dedicated issue to discuss the value of not following the OCA rule ("always use the bot to merge PRs") for the openupgrade / openupgradelib repos.
Let me know !
In the meantime, please respect the rule.

@pedrobaeza
Copy link
Member

pedrobaeza commented Jul 4, 2023

My reasons for not using ocabot here is for not wasting builds, as it's not adding value, except explicitly wanting to have CI on a rebased branch, which is not likely here most of the times. I merge the same manually when in another OCA repository, migration scripts are added. They are not being tested, and the linters are already tested on the PR.

You, being very eco-friendly with other things, should be aware of the impact of doing all that redundant builds. Maintainers and people with enough knowledge should know when they can override a guideline for the greater good.

@legalsylvain
Copy link
Contributor

Hi @pedrobaeza. thanks for your answer.

In fact, I don't understand how you can know that the merge will sucess if the pending PR is not rebased. There are tests on Openupgrade. The module installed are based on the coverage file. So if the coverage file is not up to date (that is occures, when the PR is not rebased), you cannot guarantee the success of the merge.

Well, I'm OK to stop to use the bot, as far as there is no bad side effect.
From what I understand, just because the CI has been green doesn't mean it always will be. including in this project. See for exemple : #2306 (comment)

Let me know if I miss something.

regards.

@pedrobaeza
Copy link
Member

The CI may become red in anytime due to outsider conditions, but launching ocabot on pending PRs will only highlights the inner fault on the main branch, but it's not a fault of the PR itself.

On OpenUpgrade, there can't be interactions between non dependent modules, as they incorporate isolated migration scripts, and the minimal existing tests only check specific operations done for the corresponding module.

My goal is to launch the minimal possible CI builds. If you have a PR depending on another one due to module hierarchy, there are 2 options:

  • Rebase, and after green CI, merge directly.
  • Launch ocabot command without rebasing.

On both cases, only one CI is built. The rest is redundant.

Of course this is not critical, and thousands of other CIs are launched without need, but due to the high number of PRs in this repo (this one is number 3379 - no OCA repo gets even a fraction of this number), we can make a significant difference in OCA ecosystem.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants