Open SirAionTech opened 1 month ago
The bot is not expected to convert the fragments. That's the role of pre-commit or the developer.
Thanks for having a look!
The bot is not expected to convert the fragments. That's the role of pre-commit or the developer.
Then the bot never regenerates the README? Because the linked post-merge commit (https://github.com/OCA/l10n-italy/commit/e4e5192952d8dc259f92668a3c397a43c1f708a8), and also translation ones (like https://github.com/OCA/l10n-italy/commit/0536eb0490b706cbc8c7cb690f3e6a73771d667c) all have
SKIP: oca-gen-addon-readme
I see that in the pull request CI (https://github.com/OCA/l10n-italy/blob/79f19bb8345fd5c1d04ea483b25f112269ba36fc/.github/workflows/pre-commit.yml#L38) the oca-gen-addon-readme
is skipped too.
Just looking at the configuration, it looks to me like if someone pushes a change in a README fragment without regenerating the README, the README won't be checked or updated automatically, so the regeneration is something the next developer will have to do; what am I missing?
If this is more of a repo configuration issue, feel free to move it to the https://github.com/OCA/oca-addons-repo-template repo, I opened it here because I thought the bot was misbehaving.
Just looking at the configuration, it looks to me like if someone pushes a change in a README fragment without regenerating the README, the README won't be checked or updated automatically, so the regeneration is something the next developer will have to do; what am I missing?
If yes, that's a regression. Before, the bot when merging was updating main README.rst file.
Just looking at the configuration, it looks to me like if someone pushes a change in a README fragment without regenerating the README, the README won't be checked or updated automatically, so the regeneration is something the next developer will have to do; what am I missing?
If yes, that's a regression. Before, the bot when merging was updating main README.rst file.
Now I see https://github.com/OCA/oca-github-bot/blob/2376d302f92b941e6264e3c4dc86f7703187cbf1/src/oca_github_bot/tasks/main_branch_bot.py#L37 then it shoudn't be the case.
But still, I expected the README to be regenerated in https://github.com/OCA/l10n-italy/commit/e4e5192952d8dc259f92668a3c397a43c1f708a8.
Maybe it didn't happen because the PR it came from (https://github.com/OCA/l10n-italy/pull/3599) was still using a pre-commit
configuration that used .rst
fragments and the bot is working on that branch?
I don't think there is any regression.
I understand you expect markdown conversion to be done automatically by the bot? But that's not how it is designed today. And honestly I don't think it would be a good thing to do.
I am beginning to understand the steps to have this behavior:
.rst
fragments..rst
fragments): the developer generates the README correctly..md
fragments.Expected post-merge changes convert the README
Actual
post-merge does not convert the README
side-effect: all the developers working on the repo will have pre-commit
failing and trying to add fragments conversion.
As you said, this is how it's designed now, but the side-effect is pretty annoying for any developer working on the repo.
I don't think there is any regression.
thanks for the explanation !
As you said, this is how it's designed now, but the side-effect is pretty annoying for any developer working on the repo.
It's a complicated topic. What we have today is a trade-off between developer comfort and ease of contribution to fragments by functional folks, who want to do on GitHub directly without using pre-commit.
As you said, this is how it's designed now, but the side-effect is pretty annoying for any developer working on the repo.
It's a complicated topic. What we have today is a trade-off between developer comfort and ease of contribution to fragments by functional folks, who want to do on GitHub directly without using pre-commit.
Yes, and I understand that the CI checks are relaxed because of that. But I expected the bot to do anything a developer would, including converting the README; I understand it might be risky but without it the developers have one more burden.
Anyway, I'm fixing it in the repo and hope it won't happen too often.
Yes, the bot could do the markdown conversion but
@sbidoul could you please reopen this? I can't
I reopened. It does not change my opinion that we should not do this, though ;)
Describe the bug
After a PR is merged, the bot does some post-merge changes, but the README regeneration is skipped, for instance see https://github.com/OCA/l10n-italy/commit/e4e5192952d8dc259f92668a3c397a43c1f708a8, where the pre-commit run says:
(from https://github.com/OCA/l10n-italy/actions/runs/9807706147/job/27081983177#step:7:8).
Here something in the README gets updated, but it should also rename from
.rst
to.md
the fragments in https://github.com/OCA/l10n-italy/tree/e4e5192952d8dc259f92668a3c397a43c1f708a8/l10n_it_financial_statement_eu/readmeNow the problem is that the
oca-gen-addon-readme
hook in localpre-commit
runs fails because it tries to rename the mentioned fragments.To Reproduce
Steps to reproduce the behavior:
.rst
fragments..rst
fragments): the developer generates the README correctly..md
fragments.Expected post-merge changes convert the README
Actual post-merge does not convert the README side-effect: all the developers working on the repo will have
pre-commit
failing and trying to add fragments conversion.