Page MenuHomePhabricator

Remap all submodules to tin
ClosedPublic

Authored by thcipriani on Oct 6 2015, 12:18 AM.

Details

Reviewers
dduvall
mmodell
demon
Group Reviewers
Release-Engineering-Team
Commits
rMWTS20a383d71327: Remap all submodules to tin
rMSCA20a383d71327: Remap all submodules to tin
Patch without arc
git checkout -b D9 && curl -L https://phabricator.wikimedia.org/D9?download=true | git apply
Summary

Change-Id: I905ffe975b6c37f63da7347508676b6825f90554

Test Plan

TBD

Change-Id: Ie5d3d8f3fa68d6a0190319cec7069604c671290a

Diff Detail

Repository
rMSCA Scap
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

thcipriani retitled this revision from to Remap all submodules to tin.
thcipriani updated this object.
thcipriani edited the test plan for this revision. (Show Details)
thcipriani added reviewers: mmodell, dduvall, demon.
scap/tasks.py
676

This seems to say that all submodules have to be named '/repo/modules/something' which isn't very flexible. Am I missing something?

scap/tasks.py
676

The path here is actually: [server's gitdir]/modules/[submodule-path], so, for example, to point to the restbase submodule the remote url is: http://tin/deployment/restbase/.git/modules/restbase

This is the submodule's gitdir path as of git 1.7.8 (tin is on 1.7.9): https://github.com/git/git/blob/master/Documentation/RelNotes/1.7.8.txt#L109-L114

In testing I found that, while a local submodule clone will obey a submodule .git file's gitdir: [path], a remote http submodule clone just complains that [submodule/path]/.git/ is not a directory. As such you have to update the remote's submodule path to point to the location of a submodule's actual git directory at [parent gitdir]/modules/[submodule-path]

Break apart submodule update and git checkout

The tasks.git_checkout command was previously handling submodule
checkout as well. Now that we're remapping submodules, the additional
functionality warrants its own function.

mmodell edited edge metadata.
mmodell added inline comments.
scap/tasks.py
573

I think we should switch to using get_logger() and stop using logger arguments, given how problematic the @decorator function signature stuff turned out to be. Is there an advantage to using the logger kwarg?

This is not a blocker though, I'll accept this as is.

This revision is now accepted and ready to land.Oct 7 2015, 1:00 PM
scap/tasks.py
594

Can we make this optional? There might be situations where it makes more sense to pull from gerrit (or eventually phabricator) instead of tin.

thcipriani edited edge metadata.

Make submodule mapping to tin optional

Also, reduce the use of the "logger" keyword argument

scap/tasks.py
573

One possible solution to avoid such repetition in tasks.git_* (logger, user, location etc.) and actually a lot of other tasks functions would be to introduce a basic Command or Proc class/module that encapsulates the command string, executing user, child logger, etc. There was actually a lot about checks.CheckJob and checks.execute, namely the event-loop schedule/run/poll/wait execution pattern, that I thought could live in such a module as well.

Anyway, I'm of the same mind: it's something to think about for future refactoring but not a blocker of current work.

657

We just want to process one level of submodules but won't this will process all .gitmodules files recursively?

scap/tasks.py
673

Just throwing out a straw man here but since you're strip()-ing below, you might want to just split on '='. That would catch edge cases where someone might have manually modified the .gitmodules files and used no spaces (which I think is still technically valid as far as git is concerned).

scap/tasks.py
573

I like the idea of a command / process class, and in fact I've implemented (badly) something like that in iscap. The reason for the abstraction in iscap is that I want to support executing regular shell subprocesses as well as python cli.Application instances, where the python commands can potentially run in-process instead of via the shell. The same abstraction would be really nice for remote commands which could have a local instance to represent the remote execution, making the code a lot more consistent and seamless (the remote execution would be completely abstracted away from the high-level logic)

Make submodule remap non-recursive, Add comments

git_remap_submodules now only looks for the top-level .gitmodules file
Added comments about how non-bare submodules are mapped for http remotes.

dduvall edited edge metadata.

Looks good!

This revision was automatically updated to reflect the committed changes.