feat: generate addons readme in pre-commit #190

yajo · 2023-03-28T09:06:40Z

Devs sometimes just update the readme/*.rst portions, relying in our nice bot that will convert that to a real README.rst + index.html after merge.

When functional people go test PRs in runboat, the 1st thing they look at is the README. That's OK because there's where the docs should explain what the module is doing.

These 2 situations are good in their own terms, but when you merge both, you get to a problem. You have to either:

Teach functional people to review the PR code, go to the readme/ folder and understand the .rst syntax and the advanced github diff tooling.
Teach devs to remember always using the oca-gen-addon-readme before pushing the PR, so functionals can review the README in a more human-friendly fashion.

None of these is a good solution. Both add manual and complex steps to the contribution workflow.

Quoting from OCA blog:

The Board are working on these Major Projects for 2023:
Improve Contributor onboarding journey, leverage delegates

Including Functional Consultants

This PR goes along with those goals. By moving the readme generation into a pre-commit hook:

The dev just needs to stage -> commit -> re-stage -> re-commit. Just like always.
The functionals have a nicely rendered README available right from the Odoo UI. That's the UI a functional needs to know deeply (not Github's).

@moduon

pedrobaeza · 2023-03-28T09:28:21Z

I agree with this, but I remember that there was a technical dependency for HTML generation. @sbidoul do you remember?

sbidoul · 2023-03-28T09:32:38Z

I agree with this, but I remember that there was a technical dependency for HTML generation. @sbidoul do you remember?

I've no idea what you are talking about :)

sbidoul · 2023-03-28T09:35:39Z

No, seriously, it's complicated... if we do it in pre-commit we must disable it in the bot because the bot would likely not have the same version of the generator as pre-commit. But old repos don't have pre-commit. So yeah, still not sure how to do that best. Maybe we can enable readme generation in the bot for old versions and use pre-commit for 14+? Or simply abandon readme generation for repos that don't have pre-commit.

yajo · 2023-03-28T10:10:42Z

I've no idea what you are talking about :)

Probably this: OCA/maintainer-tools#423

simply abandon readme generation for repos that don't have pre-commit.

That might be a good solution. Less tasks for the bot.

Another one is to instruct the bot to generate the readme using pre-commit run $new_module oca-gen-addon-readme.

FWIW the docutils version is pseudo-pinned.

fcvalgar · 2023-03-28T11:56:26Z

As a functional reviewer at certain times I find it very difficult to obtain information in the Readme and Usage files which are the fundamental files for us. This is increased when we are reviewing [FIX] [IMP] where there is not much information in these files.

I wish we could have access to the final file before publication.

Thank you contributors

sbidoul · 2023-03-28T12:04:12Z

I have opened OCA/maintainer-tools#564 to discuss the problem and possible solution.

At this stage I think this PR is acceptable in this template, but it should be enabled with a copier question and be disabled by default, because it would not work for OCA.

sbidoul · 2023-03-28T12:15:09Z

Ah, this has been proposed before and reverted. See the discussion there: #90

Shide

LGTM and speeds up reviewing. We are using this way of work in custom developments and helps a lot. Specially if you want to be careful about readmes

sbidoul

Sorry, I still don't think it is a good idea to enable this by default in OCA.

yajo · 2023-05-30T06:50:23Z

Hello! I've updated the PR to add --if-fragments-changed, now that OCA/maintainer-tools#520 is fixed. @sbidoul do you feel more confident now to merge this? Was that flag added too to the bot?

pedrobaeza · 2023-05-30T06:51:42Z

@sbidoul I have noticed also that the current ocabot deployed maintainer-tools seems outdated and it's not generating the links with runboat links and word.

sbidoul · 2023-05-30T09:22:06Z

It's still not clear to me how the bot and a pre-commit hook can/should interact for README generation.
it should be one or the other, otherwise we can't be sure they have the same version.

And a problem with pre-commit is that you have a red build until you run it locally, which makes it impossible tor functional contributors to work on documentation through the GitHub interface.

We need to think all this through, first.

In the meantime, if you want this pre-commit hook, it should be behind a copier question and disabled by default.

yajo · 2023-07-03T08:07:34Z

Hello @sbidoul. I pushed a fix that should resolve the situation. With the new commit, CI will skip this hook. This is a valid usage IMHO because anyways the bot will generate the README if needed. So, if a functional person pushes a change to readme/DESCRIPTION.rst and doesn't run pre-commit, the bot will re-generate README.rst. But if a dev does that locally and pushes the branch, the README.rst will probably be already generated.

Both systems should be configured with --if-fragments-changed to avoid conflicts. Here I'm doing it for the hook, but I don't know where to look for the bot configuration to see if it has that flag too.

src/.pre-commit-config.yaml.jinja

Devs sometimes just update the `readme/*.rst` portions, relying in our nice bot that will convert that to a real `README.rst` + `index.html` after merge. When functional people go test PRs in runboat, the 1st thing they look at is the README. That's OK because there's where the docs should explain what the module is doing. These 2 situations are good in their own terms, but when you merge both, you get to a problem. You have to either: - Teach functional people to review the PR code, go to the `readme/` folder and understand the `.rst` syntax and the advanced github diff tooling. - Teach devs to remember always using the `oca-gen-addon-readme` before pushing the PR, so functionals can review the README in a more human-friendly fashion. None of these is a good solution. Both add manual and complex steps to the contribution workflow. Quoting from [OCA blog][1]: > The Board are working on these Major Projects for 2023: > Improve Contributor onboarding journey, leverage delegates > - Including Functional Consultants This PR goes along with those goals. By moving the readme generation into a pre-commit hook: 1. The dev just needs to stage -> commit -> re-stage -> re-commit. Just like always. 2. The functionals have a nicely rendered README available right from the Odoo UI. That's the UI a functional needs to know deeply (not Github's). @moduon [1]: https://odoo-community.org/blog/news-updates-1/oca-newsletter-1-2023-136

yajo · 2023-09-05T09:55:16Z

This should be ready to merge.

sbidoul · 2023-09-05T11:50:51Z

CI ain't happy, though.

Now that OCA/maintainer-tools#520 is fixed, it is safer to generate readmes in the hook.

This hook is meant to run only on dev machines. On CI we can skip it because the bot will regenerate the README if needed anyway.

See https://odoo-community.org/groups/contributors-15/contributors-781433?mode=thread&date_begin=&date_end=

pedrobaeza · 2023-10-29T18:53:02Z

I see very unfrequent that two PRs modify manifest or readme at the same time, so it's not a big deal. I prefer to have this automatically generated.

legalsylvain · 2023-10-29T19:09:01Z

I see very unfrequent that two PRs modify manifest or readme at the same time, so it's not a big deal. I prefer to have this automatically generated.

As said previously, if a single PR modify readme / manifest it will conflict as soon as another PR changing the same module is merged.

PR 1 : trivial patch on any file of module A.

PR 2 : add a feature / change manifest / change readme of module A.

As soon as PR 1 is merged, PR 2 will conflict with HEAD.

pedrobaeza · 2023-10-30T07:35:34Z

AFAIK, README.rst only changes if there's a change in the contents of the readme folder, so the use case you are saying won't conflict.

sbidoul · 2023-10-30T08:35:42Z

The manifest and therefore the version is also part of the files that are hashed.
So I think Sylvain is right.

Either we disable it from pre-commit, or we add a generation mode that keeps the existing hash and we use that in pre-commit, leaving only the bot to update the hash. But that's getting complicated...

pedrobaeza · 2023-10-30T10:04:23Z

Why the manifest/version should be part of the hash if that is not reflected in the README.rst file? I think we should exclude that file from the hash.

sbidoul · 2023-10-30T10:14:46Z

That is possible too but also a brittle hack. The generation mode that keeps the existing hash and we use that in pre-commit would be better I think, but I have not thought about this in details.

yajo · 2023-10-30T12:46:45Z

It is part of the hash because the module name and summary are defined there.

pedrobaeza · 2023-10-30T12:47:57Z

OK, I see. And can this be excluded partially, or is it a whole file hash?

legalsylvain · 2023-10-30T12:50:34Z

OK, I see. And can this be excluded partially, or is it a whole file hash?

https://github.com/OCA/maintainer-tools/blob/master/tools/gen_addon_readme.py#L519

sbidoul · 2023-10-30T13:43:31Z

It is part of the hash because the module name and summary are defined there.

And maintainers.

But doing a partial hash excluding the version would be brittle (say, if someone one days adds the version to the template).

As seen in OCA#190 (comment), when somebody tried to merge 2 PRs for the same module, chances are that both imply different readme digests and the merge fails. As seen in OCA#220 (comment), this was already happening under some circumstances with `requirements.txt` files. Here I added a `.gitattributes` file which specifies a `merge=union` driver for those files. [Git docs][1] explain that this driver will just keep the changes from all files, marking the merge as succeeded and not including merge markers. This is not ideal, but is OK for our current situation because: 1. Duplicate/conflicting lines in those files should not really harm a lot. 2. The bot will re-render the README upon merge anyways, fixing that conflict automatically. 3. We could define a custom merge driver, but `union` is built into Git, meaning less configurations required downstream. Still, it doesn't seem to work in octopus merges. At least, if you do consecutive merges instead, all will work as expected. At the same time, I moved the hook to be the last. This is because previous hooks can reformat the files used to obtain the digest. [1]: https://git-scm.com/docs/gitattributes.html#Documentation/gitattributes.txt-union

As seen in OCA#190 (comment), when somebody tried to merge 2 PRs for the same module, chances are that both imply different readme digests and the merge fails. As seen in OCA#220 (comment), this was already happening under some circumstances with `requirements.txt` files. Here I added a `.gitattributes` file which specifies a `merge=union` driver for those files. [Git docs][1] explain that this driver will just keep the changes from all files, marking the merge as succeeded and not including merge markers. This is not ideal, but is OK for our current situation because: 1. Duplicate/conflicting lines in those files should not really harm a lot. 2. The bot will re-render the README upon merge anyways, fixing that conflict automatically. 3. We could define a custom merge driver, but `union` is built into Git, meaning less configurations required downstream. Still, it doesn't seem to work in octopus merges. At least, if you do consecutive merges instead, all will work as expected. At the same time, I moved the hook to be the last. This is because previous hooks can reformat the files used to obtain the digest. Closes OCA#220. [1]: https://git-scm.com/docs/gitattributes.html#Documentation/gitattributes.txt-union

This reverts commit a1551e5. Rational: The call of oca-gen-addon-readme will generate conflicts in the readme.rst file when 2 PRs modify the same module. See OCA#190 (comment)

As seen in OCA#190 (comment), when somebody tried to merge 2 PRs for the same module, chances are that both imply different readme digests and the merge fails. As seen in OCA#220 (comment), this was already happening under some circumstances with `requirements.txt` files. Here I added a `.gitattributes` file which specifies a `merge=union` driver for those files. [Git docs][1] explain that this driver will just keep the changes from all files, marking the merge as succeeded and not including merge markers. This is not ideal, but is OK for our current situation because: 1. Duplicate/conflicting lines in those files should not really harm a lot. 2. The bot will re-render the README upon merge anyways, fixing that conflict automatically. 3. We could define a custom merge driver, but `union` is built into Git, meaning less configurations required downstream. Still, it doesn't seem to work in octopus merges. At least, if you do consecutive merges instead, all will work as expected. At the same time, I moved the hook to be the last. This is because previous hooks can reformat the files used to obtain the digest. Closes OCA#220. [1]: https://git-scm.com/docs/gitattributes.html#Documentation/gitattributes.txt-union

legalsylvain · 2023-11-23T14:08:54Z

Hi @yajo. I just used this new configuration that generate an up to date readme file on OCA/16.0 PR.
It does'nt seem to work properly.

The PR is here
The autogenerated readme is here.

First problem
As you can see, the display of the images are broken because the remote is not OCA but grap (in that context) and the branch is not 16.0 but 16.0-ADD-pos_payment_method_change_policy
As a result,

I see that point as more a regression than an improvment :

before : all the images correctly displayed + readme doesn't take into account latest changes (if any)
now : all the images bad displayed + readme text take into account latest changes (if any).

Second Problem

In a git point of view, the history of the README.rst file (and index.html file) will be polluted. I mean :

commit 1 : by contributor A, break all the images url
commit 2 : by ocabot, when merging, fix all the images url
commit 3 : by contributor B, break all the images url again
commit 4 : by ocabot, when merging, fix all the images url again

Do you know how to fix it ?

CC : @sbidoul, @pedrobaeza

yajo · 2023-11-27T11:08:56Z

Thanks for the investigation 😊

About problem 1, we have the same problem still today when adding a new module whose readme contains images stored in git.

The solution would be to host those images away from git. The movements OCA seems to be doing in this regard is to allow functionals to edit those readme fragments with just markdown, and to allow drag and drop of images (and videos?). That uploads images to github CDN, which survives branch changes.

About problem 2, I don't think it's important. Besides, if problem 1 is solved, problem 2 will be also as a side effect.

legalsylvain · 2023-11-27T12:53:58Z

Thanks for your answer !

About problem 1, we have the same problem still today when adding a new module whose readme contains images stored in git.

yes. But the problem is bigger, with the recent change introduced by this PR. If now, a people make a PR against an existing module, changing the readme, all the url of images will be broken, making the readme less understandable. Before it was not the case.

The solution would be to host those images away from git

I don't know how to do it simply...

yajo · 2023-11-27T13:31:04Z

I don't know how to do it simply...

You just have to open some github issue and drag and drop the image into the comment box. Github will upload it and give you a URL. Then, remove the image from the git repo and change the URL to that one.

I agree it's not very backwards-compatible. Maybe the convert-to-markdown tool in maintainer-tools could handle this?

simahawk · 2023-12-04T16:36:04Z

FTR

FWIW I still think this feature is useless and annoying, sorry :)
I've just got again the error

Traceback (most recent call last):
  File "/home/sorsi/.cache/pre-commit/repoipl9kidr/py_env-python3/bin/oca-gen-addon-readme", line 8, in <module>
    sys.exit(gen_addon_readme())
  File "/home/sorsi/.cache/pre-commit/repoipl9kidr/py_env-python3/lib/python3.10/site-packages/click/core.py", line 1157, in __call__
    return self.main(*args, **kwargs)
  File "/home/sorsi/.cache/pre-commit/repoipl9kidr/py_env-python3/lib/python3.10/site-packages/click/core.py", line 1078, in main
    rv = self.invoke(ctx)
  File "/home/sorsi/.cache/pre-commit/repoipl9kidr/py_env-python3/lib/python3.10/site-packages/click/core.py", line 1434, in invoke
    return ctx.invoke(self.callback, **ctx.params)
  File "/home/sorsi/.cache/pre-commit/repoipl9kidr/py_env-python3/lib/python3.10/site-packages/click/core.py", line 783, in invoke
    return __callback(*args, **kwargs)
  File "/home/sorsi/.cache/pre-commit/repoipl9kidr/py_env-python3/lib/python3.10/site-packages/tools/gen_addon_readme.py", line 526, in gen_addon_readme
    gen_one_addon_readme(
  File "/home/sorsi/.cache/pre-commit/repoipl9kidr/py_env-python3/lib/python3.10/site-packages/tools/gen_addon_readme.py", line 366, in gen_one_addon_readme
    with open(template_filename, "r", encoding="utf8") as tf:
FileNotFoundError: [Errno 2] No such file or directory: '/home/sorsi/.cache/pre-commit/repoipl9kidr/py_env-python3/lib/python3.10/site-packages/tools/gen_addon_readme.rst.jinja'

pre-commit clean does not help.
Any clue?

However, from now on I will use the workaround that is actually used by many repo: set SKIP: oca-gen-addon-readme env var while committing to skip this hook.
Like: SKIP=oca-gen-addon-readme git commit -m "...whatever..."

sbidoul · 2023-12-04T16:48:38Z

@simahawk looks similar to acsone/setuptools-odoo#55 😇

Anyway given the amount of trouble people have with this feature, I'd be inclined to disable it.

pedrobaeza · 2023-12-04T16:50:46Z

Only 2 persons have commented to have troubles about this, Sylvain and Simone, so that's not a lot. And the issue functional people have reviewing PRs with an outdated README is real.

yajo · 2023-12-11T12:11:01Z

... and Simone's error is not related, BTW. It's just an installation problem.

In any case, I agree with @pedrobaeza. For us, this feature has a lot of value. It means more autonomy for the functional members of our team.

However, I'm open to other solutions for the same problem. These come to my mind:

Reopen fix(oca-gen-addon-readme): reduce unneeded merge conflicts #222 where I proposed a fix for your problem.
Rebuild READMEs within runboat process, before installing the addons.

yajo requested review from rafaelbn, sbidoul and Shide March 28, 2023 09:07

yajo self-assigned this Mar 28, 2023

yajo added the enhancement New feature or request label Mar 28, 2023

sbidoul mentioned this pull request Mar 28, 2023

[RFC] Updating the README generator automation OCA/maintainer-tools#564

Closed

Shide approved these changes Mar 29, 2023

View reviewed changes

sbidoul requested changes Mar 29, 2023

View reviewed changes

sbidoul reviewed Sep 4, 2023

View reviewed changes

src/.pre-commit-config.yaml.jinja Outdated Show resolved Hide resolved

sbidoul reviewed Sep 4, 2023

View reviewed changes

src/.pre-commit-config.yaml.jinja Show resolved Hide resolved

yajo force-pushed the gen-addon-readme branch from 5aa7529 to 69ffba5 Compare September 5, 2023 09:53

sbidoul approved these changes Sep 5, 2023

View reviewed changes

OCA-git-bot added the approved label Sep 5, 2023

yajo added 3 commits September 6, 2023 08:56

feat: add --if-source-changed to addon readme generator

ab866cb

Now that OCA/maintainer-tools#520 is fixed, it is safer to generate readmes in the hook.

ci: skip oca-gen-addon-readme

619b04f

This hook is meant to run only on dev machines. On CI we can skip it because the bot will regenerate the README if needed anyway.

build: upgrade maintainer-tools to support latest features

95c6096

See https://odoo-community.org/groups/contributors-15/contributors-781433?mode=thread&date_begin=&date_end=

yajo mentioned this pull request Oct 31, 2023

fix(oca-gen-addon-readme): reduce unneeded merge conflicts #222

Closed

legalsylvain mentioned this pull request Nov 7, 2023

[17.0] mig web_theme_classic OCA/web#2660

Merged

yajo mentioned this pull request Nov 28, 2023

[ADD] oca-gen-addon-banner: Proof of concept OCA/maintainer-tools#593

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: generate addons readme in pre-commit #190

feat: generate addons readme in pre-commit #190

yajo commented Mar 28, 2023

pedrobaeza commented Mar 28, 2023

sbidoul commented Mar 28, 2023

sbidoul commented Mar 28, 2023

yajo commented Mar 28, 2023

fcvalgar commented Mar 28, 2023

sbidoul commented Mar 28, 2023

sbidoul commented Mar 28, 2023

Shide left a comment

sbidoul left a comment

yajo commented May 30, 2023

pedrobaeza commented May 30, 2023

sbidoul commented May 30, 2023 •

edited

Loading

yajo commented Jul 3, 2023

yajo commented Sep 5, 2023

sbidoul commented Sep 5, 2023

pedrobaeza commented Oct 29, 2023

legalsylvain commented Oct 29, 2023 •

edited

Loading

pedrobaeza commented Oct 30, 2023

sbidoul commented Oct 30, 2023

pedrobaeza commented Oct 30, 2023

sbidoul commented Oct 30, 2023

yajo commented Oct 30, 2023

pedrobaeza commented Oct 30, 2023

legalsylvain commented Oct 30, 2023

sbidoul commented Oct 30, 2023

legalsylvain commented Nov 23, 2023 •

edited

Loading

yajo commented Nov 27, 2023

legalsylvain commented Nov 27, 2023

yajo commented Nov 27, 2023

simahawk commented Dec 4, 2023

sbidoul commented Dec 4, 2023 •

edited

Loading

pedrobaeza commented Dec 4, 2023

yajo commented Dec 11, 2023

feat: generate addons readme in pre-commit #190

feat: generate addons readme in pre-commit #190

Conversation

yajo commented Mar 28, 2023

pedrobaeza commented Mar 28, 2023

sbidoul commented Mar 28, 2023

sbidoul commented Mar 28, 2023

yajo commented Mar 28, 2023

fcvalgar commented Mar 28, 2023

sbidoul commented Mar 28, 2023

sbidoul commented Mar 28, 2023

Shide left a comment

Choose a reason for hiding this comment

sbidoul left a comment

Choose a reason for hiding this comment

yajo commented May 30, 2023

pedrobaeza commented May 30, 2023

sbidoul commented May 30, 2023 • edited Loading

yajo commented Jul 3, 2023

yajo commented Sep 5, 2023

sbidoul commented Sep 5, 2023

pedrobaeza commented Oct 29, 2023

legalsylvain commented Oct 29, 2023 • edited Loading

pedrobaeza commented Oct 30, 2023

sbidoul commented Oct 30, 2023

pedrobaeza commented Oct 30, 2023

sbidoul commented Oct 30, 2023

yajo commented Oct 30, 2023

pedrobaeza commented Oct 30, 2023

legalsylvain commented Oct 30, 2023

sbidoul commented Oct 30, 2023

legalsylvain commented Nov 23, 2023 • edited Loading

yajo commented Nov 27, 2023

legalsylvain commented Nov 27, 2023

yajo commented Nov 27, 2023

simahawk commented Dec 4, 2023

sbidoul commented Dec 4, 2023 • edited Loading

pedrobaeza commented Dec 4, 2023

yajo commented Dec 11, 2023

sbidoul commented May 30, 2023 •

edited

Loading

legalsylvain commented Oct 29, 2023 •

edited

Loading

legalsylvain commented Nov 23, 2023 •

edited

Loading

sbidoul commented Dec 4, 2023 •

edited

Loading