-
Notifications
You must be signed in to change notification settings - Fork 20
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
Support packages in subdirs #252
Support packages in subdirs #252
Conversation
⭐ amazing ⭐ I dunno when I'll be able to review this tbh, but I'll try to get to it within the month. |
it would be great to see this merged |
@christopher-dG, any thoughts on when you might have time to review this? (Or anyone else we could ping for review instead?) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not very familiar with python but I took a quick look through and it seems fine. And there are tests. So I vote for merging this and we can always revert if a bunch of issues come up.
I'll bring it up at SRE sync this week and see if I can put it on my schedule. If I can't find time to do it in the near future I agree with Eric, let's just send it and see what happens :) |
Any luck? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems good 👍
Closes #157.
I didn't directly follow the last proposed approach, namely "do a sweep in the registry for all packages with the same repo". Instead, I left the subpackage specification to the caller by adding an optional
subdir
input. In this way, a user can add a separate configuration blob per subpackage that they want to register. (Bonus: this terminology plays nicely with the monorepo support currently in flight for Documenter.jl). While the "register all subpackages" is potentially a follow-up use-case, this "register specific subpackages" approach is one that would suit my team's monorepo needs, and seems lean enough that it doesn't require a huge overhaul of this codebase (as was noted might be required in previous conversation).Code review notes:
In
tagbot/action/changelog.py
especially, it looks like there are a lot of changes. In practice, a certain subset of these are renaming rather than logic changes---I found that theversion
variable name was used for both the literal version of the package registered in the registry AND the value of the git tag added to a registered commit, so updated the latter to be namedversion_tag
. (Previously, these were one and the same anyway; now, the clarity helped me understand how the two things interacted.)fwiw, I struggled when adding tests due to unfamiliarity with this framework (both the mocking aspect and the Python aspect 😅). If folks have guidance for how to approach adding additional tests and/or suggestions for how to rewrite the ones I did add, I'm open!
Punch list:
v3.2.1
orMySubpackage-v3.2.1
, i.e. the version that the repo git tags know about) vs package version (3.2.1
regardless of top-level or subpackage, aka version that the registry knows/cares about)version
vars that refer to version tags rather than package versions accordingly, for claritysubdir
inputsubdir
input (no plumbing needed, uses same code asaction