Select new and modified addon compared to a git branch #20
Conversation
Codecov Report
@@ Coverage Diff @@
## main #20 +/- ##
==========================================
+ Coverage 91.00% 91.79% +0.78%
==========================================
Files 16 17 +1
Lines 489 536 +47
Branches 95 103 +8
==========================================
+ Hits 445 492 +47
Misses 25 25
Partials 19 19
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
| @@ -168,6 +188,14 @@ def callback( | |||
| # addons selection | |||
| if select_addons_dirs: | |||
| main_options.addons_selection.add_addons_dirs(select_addons_dirs) | |||
| if select_git_modified: | |||
| modified_addons = get_branch_modified_addons( | |||
| select_git_modified, main_options.addons_path | |||
There was a problem hiding this comment.
I'm not sure using the addons path here is good, as the addons path might be set by ODOO_RC, and point to several git clones. So the result might end up confusing.
To avoid an additional option to specify the git directory, we could make this look in the current directory.
There was a problem hiding this comment.
I understand the point, since you need to be somewhere in the git tree to run a command with a git option.
However, for each modified file, you have to know if it's from and Odoo module or 'something else'. So a file modified in odoo/addons/odoo_keycloak/ is in an odoo addon, but not one in doc/service/keycloak. How do you know the difference? It's supposed to be set in the addons path, and I don't see any other way to know. Just checking the local directory is a bit painful if you want next to have different subfolders to look into, and it's already a known usage pattern, so that seems like the right way to do it to me.
I understand the idea of "looking into the current folder" but it's not even how we work currently.
For now the existing commands we have on the project are all at the root of the git project and pass odoo/addons as a prefix. It would be very awkward to add cd commands before/after, and in the case of multiple directories to then stitch the results together.
|
|
||
|
|
||
| def _get_modified_files(branch: str, current_branch: str) -> List[str]: | ||
| command_modified = ["git", "diff", "--name-only", branch, current_branch] |
There was a problem hiding this comment.
Would it work too if we don't pass the current branch here ?
There was a problem hiding this comment.
Well, the first problem is that there was the debated change to 'main' as default made de facto standard by GitHub, so 'master' isn't even canonic anymore, even though it's certainly still more used. Even with that, many projects use 'dev' as main branch.
You can get the default branch on a given remote, the remote being canonically 'origin', but there is no guarantee either. Because this is intended to run on the central repo, it's also a bit weird to need to check the default remote.
All in all, it seems that makes a lot of assumptions to avoid giving a default that you can simply add to your command.
(The one you don't need to pass is actually current_branch because you can use HEAD, but we already don't ask for it (syntax would be a bit awkward too, whereas this is pretty explicit).)
src/manifestoo/git.py
Outdated
|
|
||
|
|
||
| def _get_branch_addons_by( | ||
| method: Callable[[str, str], list[str]], branch: str, addons_dirs: List[Path] |
There was a problem hiding this comment.
I'm not sure how to get tox to be happy with the typing of Callable. It seems any syntax fails in some version of python.
Interestingly enough, passing Callable without any details work in local against all python, but apparently not here.
Passing type: ignore or Any because of that is quite unpalatable, to say the least, so I'm not sure how to handle that.
There was a problem hiding this comment.
I've made the change anyway because I can't even work with this, since the linting rewrite the code every time I work on it. This feels like a rube-Goldberg machine where every part incorrects one another.
|
I think we'll revive this at some point, as the needs comes up now and then. |
|
Feel free to do so, you can re-use the code as you want 👍 I was just making some cleanup since in any case most of the problematic is too far lost in memories for me to keep this around. |
This PR adds :
Note that it uses popen instead of the git addon not to introduce any new dependency.