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

Support packages in subdirs #252

Merged
merged 27 commits into from
Aug 25, 2023
Merged

Support packages in subdirs #252

merged 27 commits into from
Aug 25, 2023

Conversation

hannahilea
Copy link
Contributor

@hannahilea hannahilea commented Dec 26, 2022

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:

  1. 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 the version 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 named version_tag. (Previously, these were one and the same anyway; now, the clarity helped me understand how the two things interacted.)

  2. 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:

  • Understand where "version" means tag version (aka v3.2.1 or MySubpackage-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)
    • Rename version vars that refer to version tags rather than package versions accordingly, for clarity
  • action: add new subdir input
    • Plumb it through
  • local: add new subdir input (no plumbing needed, uses same code as action
  • test - NOTE: I added some, but probably would benefit from more! See note.
    • tests pass
  • Update documentation
  • lint pass
  • open for review

@christopher-dG
Copy link
Member

⭐ amazing ⭐

I dunno when I'll be able to review this tbh, but I'll try to get to it within the month.

@CarloLucibello
Copy link

it would be great to see this merged

@hannahilea
Copy link
Contributor Author

@christopher-dG, any thoughts on when you might have time to review this? (Or anyone else we could ping for review instead?)

Copy link
Member

@ericphanson ericphanson left a 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.

@christopher-dG
Copy link
Member

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 :)

@ericphanson
Copy link
Member

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?

Copy link
Member

@christopher-dG christopher-dG left a comment

Choose a reason for hiding this comment

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

Seems good 👍

tagbot/action/repo.py Outdated Show resolved Hide resolved
tagbot/action/changelog.py Outdated Show resolved Hide resolved
tagbot/action/repo.py Outdated Show resolved Hide resolved
@christopher-dG christopher-dG merged commit 9dadb95 into JuliaRegistries:master Aug 25, 2023
3 checks passed
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.

Support packages in subdirs
4 participants