Revert "feat: generate addons readme in pre-commit" #220
Conversation
|
As you can see in #190 (comment), that PR was solving a different issue, which is also important. We can revert it until we get to a better solution, but still we'll have the original problem. I can imagine a possibly better approach: to add a According to the docs, that would ignore any merge conflicts in those files. Then, the OCA bot can re-generate the README after merging, which is something it would do anyway, since it would bump the manifest version and thus violate the hash written in the README. |
Well. i'm not see the two issues at the same level :
I don't have a clear opinion about alternatives, (and consequences). |
|
However, both issues aren't as easy to fix:
|
There was a problem hiding this comment.
Well. i'm not see the two issues at the same level :
* generate conflicts between independant PRs : **regression** * generate automatically the last up to date README.rst file that could be read by functional people on runboat : **nice to have feature**
I agree w/ @legalsylvain
I know we can disable it by repo but then... what's the purpose?
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
Sorry, I missed that point. @simahawk How it is possible to disable it by repo ? I did'nt saw the option in the PR #190. could you help me ?
I don't get it. Let's take an exemple.
As I don't have write access on etobella repo, I can't fix the branch, and so I can't use it anymore, until etobella rebase and push.
In general, I like this philosophy of "dealing with problems when they become real". |
|
But as said, if this change is already some time on OCA and haven't arisen yet, it's because yours pre-thoughts are not really like that. I would like to have a real case to see the impact. |
But I don't see this patches deployed since a month on OCA repos, but since 3 days. This patch target V14 / V15 / V16. So it should be deployed on 200 repo x 3 versions = 600 branches.
Did I missed something ? |
|
OK, the merge was done a lot of time ago, but not the deployment. Anyway, the deployment is done, and if you revert this, another massive update should be done, so let's wait for the first conflict to arise to see the consequences. |
https://github.com/OCA/edi-framework/blob/16.0/.github/workflows/pre-commit.yml#L30
But to save what? To gain what? TBH I don't understand why this is an interesting feature. Getting to "experience": personally I only had issues so far in the repo where is has been enabled due to missing update of whatever library or version... can't remember now. I ended up committing w/ My $0.02 😁 |
|
Some points to consider.
That's one of the unsatisfactory solutions mentioned in #190 (comment). We are used to OCA, Odoo, Markdown and Github... we are even used to how the bot auto-generates readmes... however, amount of corner cases that we deal with in our minds when we do that is a huge entry barrier for functional contributors. Functional contributors do this:
We have to understand that an excellent QA reviewer can be a person that will never ever know how to write a line of Markdown or RST. That's the situation I was trying to fix. If our tooling scares away excellent QA contributors, the problem is on our tooling. #190 made this scenario possible. It's just part of an effort that affected many parts of OCA, aiming to improve the contribution experience for non-techies. Some links to consider:
OTOH, I understand the problem you guys mention. It's not like I don't think it should be fixed, but certainly I wouldn't like to harm our functional team workflow yet again, after all this work is done. Keep in mind that we already had that problem when, for example, you were merging 2 PRs that changed 2 different addons that altered external_dependencies:python. The There's a solution already. Long time ago, I changed my git-aggregator configs like this: l10n-spain:
defaults:
depth: $DEPTH_MERGE
remotes:
origin: https://github.com/moduon/l10n-spain.git
upstream: https://github.com/OCA/l10n-spain.git
target: origin $ODOO_VERSION
merges:
- origin $ODOO_VERSION
- - upstream refs/pull/2979/head
+ shell_command_after:
+ - curl -sSL https://github.com/OCA/l10n-spain/pull/2979.patch | git am -3 --keep-non-patch --exclude '*requirements.txt'This resulted in less random errors and less conflicts when Please hold on while I try the solution from #220 (comment). Maybe that works and the problem just disappears. |
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
|
Please see #222. |
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
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
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
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
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)
legalsylvain
force-pushed
the
master-revert-oca-gen-addon-readme
branch
from
397057b
to
2ec4122
Compare
|
Definitively, call generate-addons-readme during pre-commit generate a lot of problem for developers. Scenario :
Then, if you to reorder your commit with a rebase to put B before A, here is the final content of README.rst file : Problem 1 : all commits are poluted by systematic two changes in README.rst and index.html file now. Please, revert accept that revert, and find another solution. The current design is not good. CC : @sbidoul |
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 #190 (comment)