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

Add SUBPROJECT_ID setting #213

Merged
merged 15 commits into from
Aug 15, 2023
Merged

Add SUBPROJECT_ID setting #213

merged 15 commits into from
Aug 15, 2023

Conversation

ewjoachim
Copy link
Member

@ewjoachim ewjoachim commented Jul 23, 2023

Fixes #16
Needs #184 ✔️
Renders #180 obsolete (sorry)

I've finally taken the time to redo my PR for allowing monorepo settings.

I'm introducing a single setting (SUBPROJECT_ID) that does multiple things:

  • changes the marker in the comment to allow multiple comment not to overwrite one another
  • changes the name of the file we write as output to avoid clashes
  • changes the name of the data branch.

Of course, it will make a lot more sense after we merge #184 and add COVERAGE_PATH (though I believe there may be cases where we may want the 2 settings to be independant: it's not unimaginable to have the coverage run on the root for both projects. It's also reasonable to use COVERAGE_PATH outside of a monorepo setting (e.g. if using src/) so while the doc for SUBPROJECT_ID mentions COVERAGE_PATH, I don't believe the 2 settings will be linked in the code.

Full backwards compatibility to be expected.

@github-actions
Copy link

End-to-end public repo
End-to-end private repo
If the tests fail, @ewjoachim will be added to the private repo

@github-actions
Copy link

github-actions bot commented Jul 23, 2023

Coverage report

The coverage rate went from 100% to 100% ➡️
The branch rate is 100%.

100% of new lines are covered.

Diff Coverage details (click to unfold)

coverage_comment/github.py

100% of new lines are covered (100% of the complete file).

coverage_comment/settings.py

100% of new lines are covered (100% of the complete file).

coverage_comment/main.py

100% of new lines are covered (100% of the complete file).

coverage_comment/template.py

100% of new lines are covered (100% of the complete file).

@ewjoachim
Copy link
Member Author

Shall we ?

@kieferro
Copy link
Member

kieferro commented Aug 5, 2023

Shall we ?

Oh, I'm sorry. I didn't realise that this was already review/merge ready. I thought you were still working on it because you never requested a review and because in some places the necessary changes have not yet been made. For example, here and also at another place the used marker is not the formated, but still the old one (Or am I completely off track?). And I also noticed something else that I thought is still missing. So I'm sorry if you've been waiting for my review all this time.

@ewjoachim
Copy link
Member Author

Oh, don't worry, no it's probably me :) I'll self-review and ping you when it's ready !

@ewjoachim ewjoachim force-pushed the monorepo branch 4 times, most recently from 8cbb0b5 to 56f2ceb Compare August 5, 2023 14:53
@ewjoachim
Copy link
Member Author

I've re-read all and added tests.

@ewjoachim ewjoachim force-pushed the monorepo branch 2 times, most recently from 8ed74f3 to c180879 Compare August 6, 2023 09:09
Copy link
Member

@kieferro kieferro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That looks great (:clap:). Very nice that we will soon be able to support monorepos with it.
I've tested it fairly extensively in a test repo with multiple IDs in PR and push mode. And the code seems to run smoothly as far as I've seen.
I still had a few minor comments and I took the liberty of fixing the e2e tests that were failing.

I think it might be a good idea if we could somehow integrate the ID into readme.md.j2 as well. Maybe even in the heading, so that the projects are easier to distinguish when looking at the README on the coverage data branch.

Otherwise, everything looks good 👍

README.md Outdated Show resolved Hide resolved
action.yml Outdated Show resolved Hide resolved
coverage_comment/settings.py Outdated Show resolved Hide resolved
@kieferro
Copy link
Member

kieferro commented Aug 6, 2023

/invite

@github-actions
Copy link

github-actions bot commented Aug 6, 2023

End-to-end private repo

@ewjoachim
Copy link
Member Author

I think it might be a good idea if we could somehow integrate the ID into readme.md.j2 as well. Maybe even in the heading, so that the projects are easier to distinguish when looking at the README on the coverage data branch.

Good idea !

@ewjoachim
Copy link
Member Author

I think we're good 🎉

@kieferro
Copy link
Member

Yes, looks great. Since we're both happy with it, I'll merge

@kieferro kieferro merged commit 64aebb0 into v3 Aug 15, 2023
1 check passed
@kieferro kieferro deleted the monorepo branch August 15, 2023 22:36
@kieferro kieferro mentioned this pull request Sep 13, 2023
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 this pull request may close these issues.

Multi-module monorepo support?
2 participants