Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Multi-module monorepo support? #16

Closed
xSAVIKx opened this issue Jun 1, 2022 · 11 comments · Fixed by #213
Closed

Multi-module monorepo support? #16

xSAVIKx opened this issue Jun 1, 2022 · 11 comments · Fixed by #213

Comments

@xSAVIKx
Copy link

xSAVIKx commented Jun 1, 2022

Soo, I spent quite some time trying to make the action work with our private monorepo setup and looks like I failed :-(

The problem is that when in monorepo, you most probably are running the pytest/coverage inside some sub-module. And thus even with relative files, the paths are relative to that submodule.

So, when being merged, you kinda lost the "submodule" info and now when running the action in the repo root, it rightfully complains about a missing .py file. The file is present, but just inside some of the sub-modules.

If there are any ideas/examples of monorepo setups, I'd be glad to see any suggestions.

Also, while the whole sub-module thing (at least in our case) has a root-level Poetry project, when running the same coverage combine or coverage json locally under a particular poetry shell, it works fine! I believe the reason for that is cause all the sub-modules are listed in the root project as dependencies and poetry resolves these quite well.

@xSAVIKx
Copy link
Author

xSAVIKx commented Jun 1, 2022

I have tried to replace the combine/json step with one where I run these commands outside of the container (and it works just fine) and then load the .json report.

But then I'd also have to move the diff generation out and this kinda has not a lot of sense then :-)

@ewjoachim
Copy link
Member

Thanks for your report. I should really spend some more time maintaining the project. I don't want to leave this ticket unanswered but I can't promise anything on when I'll have time to work on that. In the meantime, if you want to investigate the matter, feel free to put some info or questions here and I'll try to answer those (no promises).

@ewjoachim
Copy link
Member

So would there be an option that would make it work for you ? For example, providing a path, and the whole action would run in that path inside the repo ?

@xSAVIKx
Copy link
Author

xSAVIKx commented Jun 25, 2022

Well, it may help if we wanted to have a comment for a particular single module.

But what I'd really like to have is a way of reporting the whole project (multiple modules in different folders with their own structure) coverage.

@ewjoachim
Copy link
Member

So if you could configure the action to run on a specific folder, you could run the action multiple times in your workflow ?

@xSAVIKx
Copy link
Author

xSAVIKx commented Jul 2, 2022

Well, that may allow you to have multiple comments for each of the modules, right?

But ideally I'd like to have a single comment for these different modules. And the problem I face now is the loss of the knowledge about internal monorepo structure. I.e. that each gathered coverage file has relative paths, but they are relative to the modules within the monorepo structure. Ideally there should be a way to somehow say what the modules were. IDK if that's to clarify the problem anyhow 🙂

@ewjoachim
Copy link
Member

Well, that may allow you to have multiple comments for each of the modules, right?

Yes. You could have different titles for each comment.

I'm not sure I'm willing to add a big layer of complexity in the way the action arguments are defined to support this usecase (because you would then have, say, different values for red/orange/green depending on the repo, maybe different badges too etc.). I'm afraid this would etiher change a large part of the way the action works for everyone who doesn't need something this complicated, or it would make the code of the action much more complicated to support both "simple" mode and "monorepo" mode. I could change my mind if you have new arguments in mind.

@xSAVIKx
Copy link
Author

xSAVIKx commented Jul 4, 2022

I guess having at least multiple comments solution is already much better :-)

One idea though, can it be a single merged comment instead of multiple comments? I.e. some append-only log or smth

@ewjoachim
Copy link
Member

ewjoachim commented Jul 4, 2022

but then if you run the CI twice (e.g. re-push), you think it would be ok to keep both the latest result and older results in the comment ?

Otherwise, I'd guess we would need to write the run id in the comment marker or something. Assuming it would be the same run id for all the comments genearted by a single push...?

@xSAVIKx
Copy link
Author

xSAVIKx commented Jul 4, 2022

I guess ideally the action should take care of updating only the relevant part. So append only for a single run. Probably just overwriting the previous run comment is Ok

@ewjoachim
Copy link
Member

I think it wasn't obvious in my comment but I tried to shed a light on the fact that what you're asking for could be quite complicated.

There could be ways to do this, but that would be adding quite some complexity to the action. This means we should be able to not only write a comment but also parse it, or that we're able to tell when the comment has been written for a previous run and we need to overwrite it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants