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
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

thcipriani updated this revision to Diff 31.Oct 6 2015, 12:18 AM
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.
greg added a subscriber: greg.Oct 6 2015, 6:03 AM
mmodell added inline comments.Oct 6 2015, 9:06 AM
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?

thcipriani added inline comments.Oct 6 2015, 2:17 PM
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]

thcipriani updated this revision to Diff 33.Oct 6 2015, 5:53 PM

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 accepted this revision.Oct 7 2015, 1:00 PM
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
mmodell added inline comments.Oct 7 2015, 1:05 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 updated this revision to Diff 46.Oct 8 2015, 7:47 PM
thcipriani edited edge metadata.

Make submodule mapping to tin optional

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

dduvall added inline comments.Oct 8 2015, 11:04 PM
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?

dduvall added inline comments.Oct 8 2015, 11:13 PM
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).

mmodell added inline comments.Oct 9 2015, 9:47 PM
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)

thcipriani updated this revision to Diff 50.Oct 11 2015, 5:40 PM

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 accepted this revision.Oct 14 2015, 4:18 PM
dduvall edited edge metadata.

Looks good!

This revision was automatically updated to reflect the committed changes.
Restricted Application added a reviewer: Release-Engineering-Team. · View Herald TranscriptFeb 23 2018, 11:43 AM
Restricted Application added a project: Release-Engineering-Team. · View Herald Transcript