Conversation
|
View this pull request in Lando to land it once approved. |
cgsheeh
left a comment
cgsheeh
left a comment
There was a problem hiding this comment.
LGTM but want to clarify if we should apply this to git_repo also.
| "pull_path": str(git_repo), | ||
| "push_path": str(git_repo), | ||
| "required_permission": SCM_LEVEL_3, | ||
| "system_path": repos_dir / "git_repo", |
There was a problem hiding this comment.
Looks like we also hard-code the repo name as git_repo in the git_repo fixture, should this be resolved there as well? Seems that the pull_path and push_path will be .../git_repo as a result.
There was a problem hiding this comment.
Yes actually working on that now, we likely need to randomize the actual (remote) git_repo path.
zzzeid
changed the title
zzzeid
force-pushed
the
zeid/bug-2022618-repo-mc-fix
branch
from
e2c4cbb to
f5ea52b
Compare
zzzeid
force-pushed
the
zeid/bug-2022618-repo-mc-fix
branch
from
f5ea52b to
457e3c1
Compare
| revision_id = revision_id or (last_revision.id + 1 if last_revision else 1) | ||
| revision = Revision.objects.create( | ||
| revision_id=revision_id, | ||
| diff_id=revision_id * 10, |
There was a problem hiding this comment.
Can you add a comment explaining why we multiply by 10?
Why have we split out revision_id from number? Any reason they couldn't stay as a single kwarg, and if a user wants to use a non-standard revision/patch, they can just pass patch in the caller?
| epoch = "1970-01-01T00:00:00" | ||
| monkeypatch.setenv("GIT_COMMITTER_DATE", epoch) | ||
|
|
||
| repo_dir = tmp_path / str(uuid.uuid4()) |
There was a problem hiding this comment.
Could we have something more deterministic than a UUID here? This would be easier when debugging failing tests and looking in /tmp.
Maybe just a monotonically incrementing number. We'd risk more collisions, but it would be caught by the code just below (or, alternatively, we could keep a static variable somewhere to keep count of how many repos were created).
| def make_commit_maps(make_git_repo: Callable) -> Callable: | ||
| def _make_commit_maps(git_repo=None): | ||
| git_repo = git_repo or make_git_repo() |
There was a problem hiding this comment.
Should the make_git_repo fixture be one of the arguments of make_commit_maps now?
| landing_worker_instance: Callable, | ||
| make_landing_job: Callable, | ||
| commit_maps: list[CommitMap], | ||
| make_commit_maps: Callable, |
There was a problem hiding this comment.
+1 on the naming. We have been a bit wobbly (or maybe just me?) on naming fixtures that return objects vs. those that return Callables. Making the latter start with active verbs make it much clearer!
| @pytest.fixture | ||
| def git_repo(make_git_repo: Callable) -> Path: | ||
| return make_git_repo() |
There was a problem hiding this comment.
We could leave this one in conftest? Unless the intent is to fence off and deprecate its use?
3 participants
No description provided.