Repo: add commit_map_name to specify which mapping to look up (bug 2050335)

[shtrom]

• repo: add commit_map_name
• CommitMap: remove unused get_git_repo_name method
• tests: add kwargs to *repo_mc fixtures
• CommitMap: honour commit_map_name from repo, if found
• migrate_data: add 2_bug2050335_set_repo_commit_map_name
• try_api: use Repo.commit_map_name instead of TRY_REPO_MAPPING
• uplift_worker: log exception when failing to create Try job
• Makefile: add migrations-show and migrations-rollback targets
• CommitMap: absorb get_commit_map_name and get_commit_hash from try_api.api

---

This will require a data migration to be run after deployment:

lando migrate_data 2_bug2050335_set_repo_commit_map_name

---

Lando: link
Bugzilla: bug 2050335

⚠️ This pull request has 5 warnings.
🚫 This pull request has 1 blocker.

[github-actions]

View this pull request in Lando to land it once approved.

[zzzeid]

r+ but with a "nit+" that would be good to implement before this is deployed. Also a comment/question.

[zzzeid]

Could we not achieve a similar outcome by changing this to "try-comm-central": "thunderbird-desktop"?

[shtrom]

That's the approach I took initially, in #1271, but we thought that migrating the existing CommitMap data to change all the columns may not be desirable #1271 (comment).

Instead, this PR works around removal of some hard-coded stuff, which I think is more desirable.

But happy to discuss if you'd prefer to go the way of #1271 instead.

[zzzeid]

nit+: It would be cleaner to use git_repo_name here, turn it into an actual DB field that is set automatically upon first save (maybe rename the existing one to get_git_repo_name). This will eliminate the need for most of the functionality in the data migration script.

This also needs a default ("").

[shtrom]

We could do that as a default, that we could override. The main thing of having a separate field is that we don't need the TRY_REPO_MAPPING... which makes me realise I forgot to remove...

[shtrom]

One issue in using a generic such as git_repo_name rather than making it obvious that's it's for commit mapping is that it may turn into a footgun where most of the time you'll have the git_repo_name you want, and every now and then (try repos), you'll have a different one.

[shtrom]

Also, even Hg repos need a commit-mapping name, and git_repo_name rightfully raises an exception there.

[zzzeid]

I think naming aside, it would still make sense to set something on save, especially since as it is, we'll need to set this explicitly every time we create a new repo. So maybe middle ground is: keep naming as-is, but have a default set on first save, if not set already?

commit_map_name is fine, maybe commit_map_repo_name to be more explicit but both are OK.

[shtrom]

We don't need to set something explicitly, as we have defaulting behaviour in try_api.get_commit_map https://github.com/mozilla-conduit/lando/pull/1322/changes#diff-af456f968caba867ca1e61b5693012347b03c6912c7cc03dd3d86277ee410091R53-R59

However, this no longer look like the right place for this method, so I'm going to move it to the CommitMap, as it's also used by other things (such as Try Uplifts).

I don't think we can avoid the manual step here, if we want it to be meaningful: any repo that we create that really needs that info, e.g., try repos, will be using an arbitrary name that cannot be determined by looking at the current repo name (try -> firefox, try-comm-central -> thunderbird, try-nss -> nss).

With the get_commit_map logic (name liable to change after I attack this), we don't have a default written in the DB that's incorrect, though we have a defaulting behaviour that will use the git_repo_name if the repo is git, or fail if not (because we can't guess what the mapping name would be). This is with the note that, really, we would only be using this

1. for Hg target_repos
2. for which we would have manually recorded a commit_map
   because with git-to-git, we don't need commit-mapping

   lando/src/lando/try_api/api.py

   Lines 177 to 179 in c42e45b

   	if patches_request.base_commit_vcs != repo.scm_type:
   	try:
   	mapping_repo = get_commit_map(repo)

   , and I doubt we'll do hg-to-git anytime soon (really, I hope never 😅).

[shtrom]

@zzzeid more changes on this PR than I initially thought, so worth a second look before landing, on the assumption that we prefer this approach over #1271.

[zzzeid]

Rest of the changes look OK.

[zzzeid]

I think naming aside, it would still make sense to set something on save, especially since as it is, we'll need to set this explicitly every time we create a new repo. So maybe middle ground is: keep naming as-is, but have a default set on first save, if not set already?

commit_map_name is fine, maybe commit_map_repo_name to be more explicit but both are OK.

[shtrom]

Ok, good for yet another look.

I think I went in the exact opposite way than what you were describing, but I think it puts more of the related logic together.

My running assumption is that the git_repo_name in the CommitMap is a misnomer, and names the map rather than the repo. There is a tight link between various repos and the map they use, to the point that some CommitMaps are named after the git_repo_name (but not all, because I named the thunderbird-desktop map thunderbird, and all of this is very self-inflicted).

With this assumption in place, we now have logic in the CommitMap to get the commit map name associated to a repo (this was previously a utility function in the try_api), with commit_map_name = CommitMap.get_commit_map_name(repo). We can then use the other utility function extricated from the try_api and spliced into the CommitMap to do the hash resolution: hash = CommitMap.get_commit_hash(commit_map_name, hash, scm_type).

So this doesn't rename the field, nor gives it a non-empty default. Instead, it encapsulates the resolution logic within the CommitMap, with sane defaulting logic.

The only time when we'd need to explicitly set the commit_map_name on a repo is when we create a new Hg repo in Lando synced from a Git repo. I don't expect this to happen.