fix(oca-gen-addon-readme): reduce unneeded merge conflicts #222
Conversation
| @@ -235,3 +229,12 @@ repos: | |||
| args: | |||
| - --rcfile=.pylintrc-mandatory | |||
| {%- endif %} | |||
| - <<: *maintainer_tools | |||
There was a problem hiding this comment.
not sure to understand this syntax.
There was a problem hiding this comment.
It's reusing the anchor defined above. Check this out: https://www.linode.com/docs/guides/yaml-anchors-aliases-overrides-extensions/
|
Thanks a lot for your investigation and your PR ! I'll try this code in the oca days. |
yajo
force-pushed
the
no-readme-generator-conflicts
branch
2 times, most recently
from
156fc10
to
2b572e2
Compare
yajo
force-pushed
the
no-readme-generator-conflicts
branch
7 times, most recently
from
0eded17
to
3c606b2
Compare
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
Just install all required pythons and let pytest-xdist handle parallelization by itself.
yajo
force-pushed
the
no-readme-generator-conflicts
branch
from
1cd51e7
to
cd3af75
Compare
yajo
force-pushed
the
no-readme-generator-conflicts
branch
from
fb50bab
to
453081d
Compare
|
Hi there. This one is ready to merge. I was having problems because the tests were run under python 3.8 but pre-commit needed python 3.6. I had to pin an older virtualenv version because the latest one doesn't support python 3.6 anymore. Long story short, now we don't issue a matrix of builds. Instead, we install all required python interpreters in a single build and parallelize at pytest level with pytest-xdist. Thus, now there's just one "test" job. Since there's just that job, the CI checks will stay as "Required statuses must pass before merging" because I changed the way CI is handled here. If nobody's against this, I'll merge it so we can unblock the situation explained at #220 (comment). |
|
@legalsylvain have you tested this? |
|
I'm proposing using OCA/maintainer-tools#588 as an alternative. |
|
I still think this is a better solution. Multiple PRs can modify instructions in the readme folder. If those conflict, there's no other solution than to solve it. But if the conflict happens in the README, we shouldn't care when merging, as the bot would re-trigger the generation after merge. Also this PR refactors the test suite in a more future-proof way (that can be extracted separately, of course). So IMHO this still has its benefits and we should merge this. |
👍 for that Regarding the union merge, I think it may be fine for requirements.txt (although I have not personally witnessed such conflicts in practice). For README.rst/index.hml, I'm not sure. The drawbacks you mention in the OP don't seem to be worth the gain. |
|
OK, nevermind. We can come back to this later if needed. |
As seen in #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 #220 (comment), this was already happening under some circumstances with
requirements.txtfiles.Here I added a
.gitattributesfile which specifies amerge=uniondriver for those files. Git docs 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:unionis 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 #220.