headless_api: add merge-remote action (bug 1980701)#516
headless_api: add merge-remote action (bug 1980701)#516jcristau wants to merge 2 commits intomozilla-conduit:mainfrom
Conversation
jcristau
force-pushed
the
merge-remote
branch
from
0ae13a7 to
6d0c8c4
Compare
This adds a merge-remote action to the headless API, that takes a git repository URL and commit hash to merge into the target repo. The intended use case is importing code and its history from an existing repository, such as when https://github.com/mozilla/application-services will be merged into https://github.com/mozilla-firefox/firefox.
jcristau
force-pushed
the
merge-remote
branch
from
c0a1008 to
e0c23dd
Compare
|
One thing that we should be careful with merges of native git branches is that cinnabar doesn't support signed commits. If we allow any signed commit to land into a repo synced to HgMO with git-hg-sync, this will immediately create a divergence which will be painful to resolve. I think it might be worth adding a dedicated landing check that would ensure that none of the commits are signed before pushing. -> https://bugzilla.mozilla.org/show_bug.cgi?id=1996492 |
|
Unfortunately, it seems all commits in application-services are signed, so we'll have to make sure to strip the signatures. 
|
cgsheeh
left a comment
cgsheeh
left a comment
There was a problem hiding this comment.
Looking very good, I think once we have some more test coverage and docs for the ALLOWED_MERGE_REMOTE_REPOS setting this will be good to go. 👍
|
|
||
| HTTP_USER_AGENT = f"Lando/{version} ({ENVIRONMENT})" | ||
|
|
||
| ALLOWED_MERGE_REMOTE_REPOS = os.getenv("ALLOWED_MERGE_REMOTE_REPOS", "").split(",") |
There was a problem hiding this comment.
Can we document what values we might expect here? Should this be full paths to repo URLs (https://github.com/mozilla/repo), owner/repo Github paths (mozilla/repo), or are we assuming the local repo has had a git remote add repo-remote https://github.com/mozilla/repo and the value here is repo-remote?
|
|
||
|
|
||
| class MergeRemoteAction(Schema): | ||
| """Merge changes from a remote repository""" |
There was a problem hiding this comment.
| """Merge changes from a remote repository""" | |
| """Merge changes from a remote repository.""" |
| scm = repo.scm | ||
| scm.push = mock.MagicMock() | ||
|
|
||
| settings.ALLOWED_MERGE_REMOTE_REPOS = [repo.path] |
There was a problem hiding this comment.
I think we should add a test that asserts the merge is rejected on a repo name that isn't in the ALLOWED_MERGE_REMOTE_REPOS setting. If someone removed that check it wouldn't show up in the tests at the moment. :)
@shtrom filed https://bugzilla.mozilla.org/show_bug.cgi?id=1996492 to implement a check which ensures signed commits aren't merged into the repo. |
|
Thanks for the reviews. Out of curiosity, is there a reference for the issue with cinnabar and signed commits? |
From my end, nothing but conversations with @glandium and pain when rolling out https://github.com/mozilla-conduit/git-hg-sync. I could perhaps add a warning here: https://moz-conduit.readthedocs.io/en/latest/git-hg-sync.html |
To be pedantic: it ignores the signatures. So when converting to mercurial, the signature is lost. This means on a conversion back to git from mercurial without cinnabar metadata, the resulting git commit is different. With preserved cinnabar metadata, signed commits should be handled just fine. Things get more complicated when identical signed and unsigned git commits exist (same parent, same commit message, same content), because cinnabar can't handle the situation as both commits will map to the same mercurial changeset. |
|
4 participants
This adds a merge-remote action to the headless API, that takes a git repository URL and commit hash to merge into the target repo. The intended use case is importing code and its history from an existing repository, such as when https://github.com/mozilla/application-services will be merged into https://github.com/mozilla-firefox/firefox.