-
Notifications
You must be signed in to change notification settings - Fork 48
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 subdirectory registration functionality #287
Add subdirectory registration functionality #287
Conversation
@nkottary can you review this? @GunnarFarneback it would be great if you could also review this? |
Update: I've verified that the approval process now works, and that the tag created is in the format discussed in #230, i.e. {Package}-v{version}. If the subdir argument is not used, the tag remains v{version} |
Spun up a registry and repo to do some testing, looks like everything is working pretty well:
Gonna flip this from draft to normal PR as I think it's ready for review Edit: |
src/commentbot/verify_projectfile.jl
Outdated
@@ -84,5 +87,5 @@ function verify_projectfile_from_sha(reponame, sha; auth=GitHub.AnonymousAuth()) | |||
end | |||
end | |||
|
|||
return project, t.sha, projectfile_found, projectfile_valid, err | |||
return project, sha, projectfile_found, projectfile_valid, err |
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.
The returned sha
is only correct for the subdir
case. Otherwise the input commit sha
is returned rather than the tree sha for the top directory.
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.
Ah, good catch! I fixed it here: e39cc8d
Okay, I spun up my registry server again to test the updates to the SHA for top-level repo packages as well as the updated comments from the commentbot, looks good to me.
|
src/commentbot/verify_projectfile.jl
Outdated
@debug("Getting tree object for sha") | ||
t = tree(reponame, Tree(gcom.tree); auth=auth) | ||
t = tree(reponame, Tree(gcom.tree); auth=auth, params = Dict(:recursive => true)) |
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.
Would it be worth only activating recursive
in the subdir case? Otherwise there is no need to transfer all the deeper data.
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.
yeah, probably good to limit the data transfer when it's not needed. Fixed in 06747a2
Incorporated the updates from JuliaRegistries/RegistryTools.jl#33 into the Manifest. Tests seem to pass, but I have still not tested with the webui, as I don't have that spun up on my own registrator server. I think everything should still work on there as the subdir option has a default, though the webui would obviously not have the option to register a subdirectory |
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.
The parts of the PR I have any insights in look good to me.
@SebRollen Can you add some documentation on this feature in README.md? This can be a section after "Note on documentation build" in the "How to use" section |
@nkottary, added a section on subdirectory registration to the README |
Draft pull request for allowing subdirectory registration with Registrator.jl. So far, I've only tackled the registration piece from comments, but will tackle web-ui registration (if needed, I've never used it so not sure what the process looks like) and the
approval call next, but I figured I'd open this up for discussion. The keyword for registering a subdirectory is@bot register subdir=path/to/package
Resolves #230
This is currently dependent on a yet-to-be-merged branch of RegistryTools, here