Merge ~pappacena/launchpad:git-repo-async-privacy-virtualrefs into launchpad:master
- Git
- lp:~pappacena/launchpad
- git-repo-async-privacy-virtualrefs
- Merge into master
Status: | Needs review |
---|---|
Proposed branch: | ~pappacena/launchpad:git-repo-async-privacy-virtualrefs |
Merge into: | launchpad:master |
Prerequisite: | ~pappacena/launchpad:git-repo-async-provacy-garbo |
Diff against target: |
1053 lines (+547/-36) 14 files modified
database/schema/security.cfg (+1/-1) lib/lp/code/browser/tests/test_branchmergeproposal.py (+1/-1) lib/lp/code/interfaces/branchmergeproposal.py (+45/-1) lib/lp/code/interfaces/githosting.py (+8/-0) lib/lp/code/interfaces/gitrepository.py (+12/-1) lib/lp/code/model/branchmergeproposal.py (+98/-0) lib/lp/code/model/githosting.py (+29/-4) lib/lp/code/model/gitjob.py (+26/-1) lib/lp/code/model/gitrepository.py (+5/-2) lib/lp/code/model/tests/test_branchmergeproposal.py (+234/-0) lib/lp/code/model/tests/test_githosting.py (+2/-3) lib/lp/code/model/tests/test_gitrepository.py (+75/-20) lib/lp/code/subscribers/branchmergeproposal.py (+8/-2) lib/lp/code/tests/helpers.py (+3/-0) |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Launchpad code reviewers | Pending | ||
Review via email: mp+393075@code.launchpad.net |
This proposal supersedes a proposal from 2020-10-29.
Commit message
Doing virtual refs synchronization when running git repository privacy change background task
Description of the change
This branch carries code from https:/
Unmerged commits
- 0f268fd... by Thiago F. Pappacena
-
Small bug fixes on permission change job
- e954159... by Thiago F. Pappacena
-
Merge branch 'git-repo-
async-privacy' into git-repo- async-privacy- virtualrefs - 974b22a... by Thiago F. Pappacena
-
Fixing test
- 6fde200... by Thiago F. Pappacena
-
Refactoring the way we deal with copy/delete virtual refs operations
- 7793c43... by Thiago F. Pappacena
-
Merge branch 'create-mp-refs' into git-repo-
async-privacy- virtualrefs - b4af659... by Thiago F. Pappacena
-
Merge branch 'master' into create-mp-refs
- 1688ced... by Thiago F. Pappacena
-
Running the branch sync when changing repository privacy
- 6dbcc2a... by Thiago F. Pappacena
-
Merge branch 'create-mp-refs' into git-repo-
async-privacy- virtualrefs - 6118345... by Thiago F. Pappacena
-
Improving tests for more scenarios
- 20ea95f... by Thiago F. Pappacena
-
Adding garbo to keep GitRepo status in line with info type change job
Preview Diff
1 | diff --git a/database/schema/security.cfg b/database/schema/security.cfg | |||
2 | index 290e7bc..94cb548 100644 | |||
3 | --- a/database/schema/security.cfg | |||
4 | +++ b/database/schema/security.cfg | |||
5 | @@ -2083,7 +2083,7 @@ public.xref = SELECT, INSERT, DELETE | |||
6 | 2083 | type=user | 2083 | type=user |
7 | 2084 | 2084 | ||
8 | 2085 | [privacy-change-jobs] | 2085 | [privacy-change-jobs] |
10 | 2086 | groups=script | 2086 | groups=script, branchscanner |
11 | 2087 | public.accessartifact = SELECT, UPDATE, DELETE, INSERT | 2087 | public.accessartifact = SELECT, UPDATE, DELETE, INSERT |
12 | 2088 | public.accessartifactgrant = SELECT, UPDATE, DELETE, INSERT | 2088 | public.accessartifactgrant = SELECT, UPDATE, DELETE, INSERT |
13 | 2089 | public.accesspolicyartifact = SELECT, UPDATE, DELETE, INSERT | 2089 | public.accesspolicyartifact = SELECT, UPDATE, DELETE, INSERT |
14 | diff --git a/lib/lp/code/browser/tests/test_branchmergeproposal.py b/lib/lp/code/browser/tests/test_branchmergeproposal.py | |||
15 | index f2ec4ec..83c51dc 100644 | |||
16 | --- a/lib/lp/code/browser/tests/test_branchmergeproposal.py | |||
17 | +++ b/lib/lp/code/browser/tests/test_branchmergeproposal.py | |||
18 | @@ -2336,7 +2336,7 @@ class TestLatestProposalsForEachBranchGit( | |||
19 | 2336 | 2336 | ||
20 | 2337 | @staticmethod | 2337 | @staticmethod |
21 | 2338 | def _setBranchInvisible(branch): | 2338 | def _setBranchInvisible(branch): |
23 | 2339 | removeSecurityProxy(branch.repository).transitionToInformationType( | 2339 | removeSecurityProxy(branch.repository)._transitionToInformationType( |
24 | 2340 | InformationType.USERDATA, branch.owner, verify_policy=False) | 2340 | InformationType.USERDATA, branch.owner, verify_policy=False) |
25 | 2341 | 2341 | ||
26 | 2342 | 2342 | ||
27 | diff --git a/lib/lp/code/interfaces/branchmergeproposal.py b/lib/lp/code/interfaces/branchmergeproposal.py | |||
28 | index bf481c9..efe1e4a 100644 | |||
29 | --- a/lib/lp/code/interfaces/branchmergeproposal.py | |||
30 | +++ b/lib/lp/code/interfaces/branchmergeproposal.py | |||
31 | @@ -1,4 +1,4 @@ | |||
33 | 1 | # Copyright 2009-2018 Canonical Ltd. This software is licensed under the | 1 | # Copyright 2009-2020 Canonical Ltd. This software is licensed under the |
34 | 2 | # GNU Affero General Public License version 3 (see the file LICENSE). | 2 | # GNU Affero General Public License version 3 (see the file LICENSE). |
35 | 3 | 3 | ||
36 | 4 | """The interface for branch merge proposals.""" | 4 | """The interface for branch merge proposals.""" |
37 | @@ -519,6 +519,50 @@ class IBranchMergeProposalView(Interface): | |||
38 | 519 | def getLatestDiffUpdateJob(): | 519 | def getLatestDiffUpdateJob(): |
39 | 520 | """Return the latest IUpdatePreviewDiffJob for this MP.""" | 520 | """Return the latest IUpdatePreviewDiffJob for this MP.""" |
40 | 521 | 521 | ||
41 | 522 | def getGitRefCopyOperations(): | ||
42 | 523 | """Gets the list of GitHosting copy operations that should be done | ||
43 | 524 | in order to keep this MP's virtual refs up-to-date. | ||
44 | 525 | |||
45 | 526 | Note that, for now, the only possible virtual ref that could | ||
46 | 527 | possibly be copied is GIT_CREATE_MP_VIRTUAL_REF. So, this method | ||
47 | 528 | will return at most 1 copy operation. Once new virtual refs will be | ||
48 | 529 | created, this method should be extended to add more copy operations | ||
49 | 530 | too. | ||
50 | 531 | |||
51 | 532 | :return: A list of RefCopyOperation objects. | ||
52 | 533 | """ | ||
53 | 534 | |||
54 | 535 | def getGitRefDeleteOperations(force_delete=False): | ||
55 | 536 | """Gets the list of git refs that should be deleted in order to keep | ||
56 | 537 | this MP's virtual refs up-to-date. | ||
57 | 538 | |||
58 | 539 | Note that, for now, the only existing virtual ref to be deleted is | ||
59 | 540 | GIT_CREATE_MP_VIRTUAL_REF. So this method will only ever delete at | ||
60 | 541 | most 1 virtual ref. Once new virtual refs will be created, this method | ||
61 | 542 | should be extended to delete them also. | ||
62 | 543 | |||
63 | 544 | :param force_delete: True if we should get delete operation | ||
64 | 545 | regardless of any check. False otherwise. | ||
65 | 546 | :return: A list of ref names. | ||
66 | 547 | """ | ||
67 | 548 | |||
68 | 549 | def syncGitVirtualRefs(force_delete=False): | ||
69 | 550 | """Requests all copies and deletion of virtual refs to make git code | ||
70 | 551 | hosting in sync with this MP. | ||
71 | 552 | |||
72 | 553 | :param force_delete: Do not try to copy any new virtual ref. Just | ||
73 | 554 | delete all virtual refs possibly created. | ||
74 | 555 | """ | ||
75 | 556 | |||
76 | 557 | def bulkSyncGitVirtualRefs(merge_proposals): | ||
77 | 558 | """Synchronizes a set of merge proposals' virtual refs. | ||
78 | 559 | |||
79 | 560 | :params merge_proposals: The set of merge proposals that should have | ||
80 | 561 | the virtual refs synced. | ||
81 | 562 | :return: A tuple with the list of (copy operations, delete operations) | ||
82 | 563 | executed. | ||
83 | 564 | """ | ||
84 | 565 | |||
85 | 522 | 566 | ||
86 | 523 | class IBranchMergeProposalEdit(Interface): | 567 | class IBranchMergeProposalEdit(Interface): |
87 | 524 | 568 | ||
88 | diff --git a/lib/lp/code/interfaces/githosting.py b/lib/lp/code/interfaces/githosting.py | |||
89 | index 441e850..7fe0d0e 100644 | |||
90 | --- a/lib/lp/code/interfaces/githosting.py | |||
91 | +++ b/lib/lp/code/interfaces/githosting.py | |||
92 | @@ -148,3 +148,11 @@ class IGitHostingClient(Interface): | |||
93 | 148 | :param refs: A list of tuples like (repo_path, ref_name) to be deleted. | 148 | :param refs: A list of tuples like (repo_path, ref_name) to be deleted. |
94 | 149 | :param logger: An optional logger. | 149 | :param logger: An optional logger. |
95 | 150 | """ | 150 | """ |
96 | 151 | |||
97 | 152 | def bulkSyncRefs(copy_operations, delete_operations): | ||
98 | 153 | """Executes all copy operations and delete operations on Turnip | ||
99 | 154 | side. | ||
100 | 155 | |||
101 | 156 | :param copy_operations: The list of RefCopyOperation to run. | ||
102 | 157 | :param delete_operations: The list of RefDeleteOperation to run. | ||
103 | 158 | """ | ||
104 | diff --git a/lib/lp/code/interfaces/gitrepository.py b/lib/lp/code/interfaces/gitrepository.py | |||
105 | index 192133a..48e7dd3 100644 | |||
106 | --- a/lib/lp/code/interfaces/gitrepository.py | |||
107 | +++ b/lib/lp/code/interfaces/gitrepository.py | |||
108 | @@ -8,6 +8,8 @@ __metaclass__ = type | |||
109 | 8 | __all__ = [ | 8 | __all__ = [ |
110 | 9 | 'ContributorGitIdentity', | 9 | 'ContributorGitIdentity', |
111 | 10 | 'GitIdentityMixin', | 10 | 'GitIdentityMixin', |
112 | 11 | 'GIT_CREATE_MP_VIRTUAL_REF', | ||
113 | 12 | 'GIT_MP_VIRTUAL_REF_FORMAT', | ||
114 | 11 | 'GIT_REPOSITORY_NAME_VALIDATION_ERROR_MESSAGE', | 13 | 'GIT_REPOSITORY_NAME_VALIDATION_ERROR_MESSAGE', |
115 | 12 | 'git_repository_name_validator', | 14 | 'git_repository_name_validator', |
116 | 13 | 'IGitRepository', | 15 | 'IGitRepository', |
117 | @@ -105,6 +107,11 @@ valid_git_repository_name_pattern = re.compile( | |||
118 | 105 | r"^(?i)[a-z0-9][a-z0-9+\.\-@_]*\Z") | 107 | r"^(?i)[a-z0-9][a-z0-9+\.\-@_]*\Z") |
119 | 106 | 108 | ||
120 | 107 | 109 | ||
121 | 110 | # Virtual ref where we automatically put a copy of any open merge proposal. | ||
122 | 111 | GIT_MP_VIRTUAL_REF_FORMAT = 'refs/merge/{mp.id}/head' | ||
123 | 112 | GIT_CREATE_MP_VIRTUAL_REF = 'git.mergeproposal_virtualref.enabled' | ||
124 | 113 | |||
125 | 114 | |||
126 | 108 | def valid_git_repository_name(name): | 115 | def valid_git_repository_name(name): |
127 | 109 | """Return True iff the name is valid as a Git repository name. | 116 | """Return True iff the name is valid as a Git repository name. |
128 | 110 | 117 | ||
129 | @@ -595,11 +602,15 @@ class IGitRepositoryView(IHasRecipes): | |||
130 | 595 | def updateLandingTargets(paths): | 602 | def updateLandingTargets(paths): |
131 | 596 | """Update landing targets (MPs where this repository is the source). | 603 | """Update landing targets (MPs where this repository is the source). |
132 | 597 | 604 | ||
134 | 598 | For each merge proposal, create `UpdatePreviewDiffJob`s. | 605 | For each merge proposal, create `UpdatePreviewDiffJob`s, and runs |
135 | 606 | the appropriate GitHosting.copyRefs operation to allow having | ||
136 | 607 | virtual merge refs with the updated branch version. | ||
137 | 599 | 608 | ||
138 | 600 | :param paths: A list of reference paths. Any merge proposals whose | 609 | :param paths: A list of reference paths. Any merge proposals whose |
139 | 601 | source is this repository and one of these paths will have their | 610 | source is this repository and one of these paths will have their |
140 | 602 | diffs updated. | 611 | diffs updated. |
141 | 612 | :return: Returns a tuple with the list of background jobs created, | ||
142 | 613 | and the list of ref copies requested to GitHosting. | ||
143 | 603 | """ | 614 | """ |
144 | 604 | 615 | ||
145 | 605 | def markRecipesStale(paths): | 616 | def markRecipesStale(paths): |
146 | diff --git a/lib/lp/code/model/branchmergeproposal.py b/lib/lp/code/model/branchmergeproposal.py | |||
147 | index 061fb1e..821b15d 100644 | |||
148 | --- a/lib/lp/code/model/branchmergeproposal.py | |||
149 | +++ b/lib/lp/code/model/branchmergeproposal.py | |||
150 | @@ -12,6 +12,7 @@ __all__ = [ | |||
151 | 12 | 12 | ||
152 | 13 | from collections import defaultdict | 13 | from collections import defaultdict |
153 | 14 | from email.utils import make_msgid | 14 | from email.utils import make_msgid |
154 | 15 | import logging | ||
155 | 15 | from operator import attrgetter | 16 | from operator import attrgetter |
156 | 16 | import sys | 17 | import sys |
157 | 17 | 18 | ||
158 | @@ -84,7 +85,12 @@ from lp.code.interfaces.codereviewcomment import ICodeReviewComment | |||
159 | 84 | from lp.code.interfaces.codereviewinlinecomment import ( | 85 | from lp.code.interfaces.codereviewinlinecomment import ( |
160 | 85 | ICodeReviewInlineCommentSet, | 86 | ICodeReviewInlineCommentSet, |
161 | 86 | ) | 87 | ) |
162 | 88 | from lp.code.interfaces.githosting import IGitHostingClient | ||
163 | 87 | from lp.code.interfaces.gitref import IGitRef | 89 | from lp.code.interfaces.gitref import IGitRef |
164 | 90 | from lp.code.interfaces.gitrepository import ( | ||
165 | 91 | GIT_CREATE_MP_VIRTUAL_REF, | ||
166 | 92 | GIT_MP_VIRTUAL_REF_FORMAT, | ||
167 | 93 | ) | ||
168 | 88 | from lp.code.mail.branch import RecipientReason | 94 | from lp.code.mail.branch import RecipientReason |
169 | 89 | from lp.code.model.branchrevision import BranchRevision | 95 | from lp.code.model.branchrevision import BranchRevision |
170 | 90 | from lp.code.model.codereviewcomment import CodeReviewComment | 96 | from lp.code.model.codereviewcomment import CodeReviewComment |
171 | @@ -94,6 +100,10 @@ from lp.code.model.diff import ( | |||
172 | 94 | IncrementalDiff, | 100 | IncrementalDiff, |
173 | 95 | PreviewDiff, | 101 | PreviewDiff, |
174 | 96 | ) | 102 | ) |
175 | 103 | from lp.code.model.githosting import ( | ||
176 | 104 | RefCopyOperation, | ||
177 | 105 | RefDeleteOperation, | ||
178 | 106 | ) | ||
179 | 97 | from lp.registry.interfaces.person import ( | 107 | from lp.registry.interfaces.person import ( |
180 | 98 | IPerson, | 108 | IPerson, |
181 | 99 | IPersonSet, | 109 | IPersonSet, |
182 | @@ -123,6 +133,7 @@ from lp.services.database.sqlbase import ( | |||
183 | 123 | quote, | 133 | quote, |
184 | 124 | SQLBase, | 134 | SQLBase, |
185 | 125 | ) | 135 | ) |
186 | 136 | from lp.services.features import getFeatureFlag | ||
187 | 126 | from lp.services.helpers import shortlist | 137 | from lp.services.helpers import shortlist |
188 | 127 | from lp.services.job.interfaces.job import JobStatus | 138 | from lp.services.job.interfaces.job import JobStatus |
189 | 128 | from lp.services.job.model.job import Job | 139 | from lp.services.job.model.job import Job |
190 | @@ -144,6 +155,9 @@ from lp.soyuz.enums import ( | |||
191 | 144 | ) | 155 | ) |
192 | 145 | 156 | ||
193 | 146 | 157 | ||
194 | 158 | logger = logging.getLogger(__name__) | ||
195 | 159 | |||
196 | 160 | |||
197 | 147 | def is_valid_transition(proposal, from_state, next_state, user=None): | 161 | def is_valid_transition(proposal, from_state, next_state, user=None): |
198 | 148 | """Is it valid for this user to move this proposal to to next_state? | 162 | """Is it valid for this user to move this proposal to to next_state? |
199 | 149 | 163 | ||
200 | @@ -971,6 +985,7 @@ class BranchMergeProposal(SQLBase, BugLinkTargetMixin): | |||
201 | 971 | branch_merge_proposal=self.id): | 985 | branch_merge_proposal=self.id): |
202 | 972 | job.destroySelf() | 986 | job.destroySelf() |
203 | 973 | self._preview_diffs.remove() | 987 | self._preview_diffs.remove() |
204 | 988 | |||
205 | 974 | self.destroySelf() | 989 | self.destroySelf() |
206 | 975 | 990 | ||
207 | 976 | def getUnlandedSourceBranchRevisions(self): | 991 | def getUnlandedSourceBranchRevisions(self): |
208 | @@ -1217,6 +1232,89 @@ class BranchMergeProposal(SQLBase, BugLinkTargetMixin): | |||
209 | 1217 | for diff in diffs) | 1232 | for diff in diffs) |
210 | 1218 | return [diff_dict.get(revisions) for revisions in revision_list] | 1233 | return [diff_dict.get(revisions) for revisions in revision_list] |
211 | 1219 | 1234 | ||
212 | 1235 | @property | ||
213 | 1236 | def should_have_git_virtual_refs(self): | ||
214 | 1237 | """True if this MP should have virtual refs in the target | ||
215 | 1238 | repository. False otherwise.""" | ||
216 | 1239 | # If the source repository is private and it's opening a MP to | ||
217 | 1240 | # another repository, we should not risk disclosing the private code to | ||
218 | 1241 | # everyone with permission to see the target repo: | ||
219 | 1242 | private_source = self.source_git_repository.private | ||
220 | 1243 | same_owner = self.source_git_repository.owner.inTeam( | ||
221 | 1244 | self.target_git_repository.owner) | ||
222 | 1245 | return not private_source or same_owner | ||
223 | 1246 | |||
224 | 1247 | def getGitRefCopyOperations(self): | ||
225 | 1248 | """See `IBranchMergeProposal`""" | ||
226 | 1249 | if (not self.target_git_repository | ||
227 | 1250 | or not getFeatureFlag(GIT_CREATE_MP_VIRTUAL_REF)): | ||
228 | 1251 | # Not a git MP. Skip. | ||
229 | 1252 | return [] | ||
230 | 1253 | if not self.should_have_git_virtual_refs: | ||
231 | 1254 | return [] | ||
232 | 1255 | return [RefCopyOperation( | ||
233 | 1256 | self.source_git_repository.getInternalPath(), | ||
234 | 1257 | self.source_git_commit_sha1, | ||
235 | 1258 | self.target_git_repository.getInternalPath(), | ||
236 | 1259 | GIT_MP_VIRTUAL_REF_FORMAT.format(mp=self))] | ||
237 | 1260 | |||
238 | 1261 | def getGitRefDeleteOperations(self, force_delete=False): | ||
239 | 1262 | """See `IBranchMergeProposal`""" | ||
240 | 1263 | if self.source_git_ref is None: | ||
241 | 1264 | # Not a git MP. Skip. | ||
242 | 1265 | return [] | ||
243 | 1266 | if not force_delete and self.should_have_git_virtual_refs: | ||
244 | 1267 | return [] | ||
245 | 1268 | return [RefDeleteOperation( | ||
246 | 1269 | self.target_git_repository.getInternalPath(), | ||
247 | 1270 | GIT_MP_VIRTUAL_REF_FORMAT.format(mp=self))] | ||
248 | 1271 | |||
249 | 1272 | def syncGitVirtualRefs(self, force_delete=False): | ||
250 | 1273 | """See `IBranchMergeProposal`""" | ||
251 | 1274 | if self.source_git_ref is None: | ||
252 | 1275 | return | ||
253 | 1276 | if not getFeatureFlag(GIT_CREATE_MP_VIRTUAL_REF): | ||
254 | 1277 | # XXX pappacena 2020-09-15: Do not try to remove virtual refs if | ||
255 | 1278 | # the feature is disabled. It might be disabled due to bug on | ||
256 | 1279 | # the first versions, for example, and we should have a way to | ||
257 | 1280 | # skip this feature entirely. | ||
258 | 1281 | # Once the feature is stable, we should actually *always* delete | ||
259 | 1282 | # the virtual refs, since we don't know if the feature was | ||
260 | 1283 | # enabled or not when the branch was created. | ||
261 | 1284 | return | ||
262 | 1285 | if force_delete: | ||
263 | 1286 | copy_operations = [] | ||
264 | 1287 | else: | ||
265 | 1288 | copy_operations = self.getGitRefCopyOperations() | ||
266 | 1289 | delete_operations = self.getGitRefDeleteOperations(force_delete) | ||
267 | 1290 | |||
268 | 1291 | if copy_operations or delete_operations: | ||
269 | 1292 | hosting_client = getUtility(IGitHostingClient) | ||
270 | 1293 | hosting_client.bulkSyncRefs(copy_operations, delete_operations) | ||
271 | 1294 | |||
272 | 1295 | @classmethod | ||
273 | 1296 | def bulkSyncGitVirtualRefs(cls, merge_proposals): | ||
274 | 1297 | """See `IBranchMergeProposal`""" | ||
275 | 1298 | if not getFeatureFlag(GIT_CREATE_MP_VIRTUAL_REF): | ||
276 | 1299 | # XXX pappacena 2020-09-15: Do not try to remove virtual refs if | ||
277 | 1300 | # the feature is disabled. It might be disabled due to bug on | ||
278 | 1301 | # the first versions, for example, and we should have a way to | ||
279 | 1302 | # skip this feature entirely. | ||
280 | 1303 | # Once the feature is stable, we should actually *always* delete | ||
281 | 1304 | # the virtual refs, since we don't know if the feature was | ||
282 | 1305 | # enabled or not when the branch was created. | ||
283 | 1306 | return [], [] | ||
284 | 1307 | copy_operations = [] | ||
285 | 1308 | delete_operations = [] | ||
286 | 1309 | for merge_proposal in merge_proposals: | ||
287 | 1310 | copy_operations += merge_proposal.getGitRefCopyOperations() | ||
288 | 1311 | delete_operations += merge_proposal.getGitRefDeleteOperations() | ||
289 | 1312 | |||
290 | 1313 | if copy_operations or delete_operations: | ||
291 | 1314 | hosting_client = getUtility(IGitHostingClient) | ||
292 | 1315 | hosting_client.bulkSyncRefs(copy_operations, delete_operations) | ||
293 | 1316 | return copy_operations, delete_operations | ||
294 | 1317 | |||
295 | 1220 | def scheduleDiffUpdates(self, return_jobs=True): | 1318 | def scheduleDiffUpdates(self, return_jobs=True): |
296 | 1221 | """See `IBranchMergeProposal`.""" | 1319 | """See `IBranchMergeProposal`.""" |
297 | 1222 | from lp.code.model.branchmergeproposaljob import ( | 1320 | from lp.code.model.branchmergeproposaljob import ( |
298 | diff --git a/lib/lp/code/model/githosting.py b/lib/lp/code/model/githosting.py | |||
299 | index 74f4ec0..d0e23cf 100644 | |||
300 | --- a/lib/lp/code/model/githosting.py | |||
301 | +++ b/lib/lp/code/model/githosting.py | |||
302 | @@ -7,9 +7,11 @@ __metaclass__ = type | |||
303 | 7 | __all__ = [ | 7 | __all__ = [ |
304 | 8 | 'GitHostingClient', | 8 | 'GitHostingClient', |
305 | 9 | 'RefCopyOperation', | 9 | 'RefCopyOperation', |
306 | 10 | 'RefDeleteOperation', | ||
307 | 10 | ] | 11 | ] |
308 | 11 | 12 | ||
309 | 12 | import base64 | 13 | import base64 |
310 | 14 | from collections import defaultdict | ||
311 | 13 | import json | 15 | import json |
312 | 14 | import sys | 16 | import sys |
313 | 15 | 17 | ||
314 | @@ -33,7 +35,6 @@ from lp.code.errors import ( | |||
315 | 33 | GitRepositoryDeletionFault, | 35 | GitRepositoryDeletionFault, |
316 | 34 | GitRepositoryScanFault, | 36 | GitRepositoryScanFault, |
317 | 35 | GitTargetError, | 37 | GitTargetError, |
318 | 36 | NoSuchGitReference, | ||
319 | 37 | ) | 38 | ) |
320 | 38 | from lp.code.interfaces.githosting import IGitHostingClient | 39 | from lp.code.interfaces.githosting import IGitHostingClient |
321 | 39 | from lp.services.config import config | 40 | from lp.services.config import config |
322 | @@ -55,12 +56,21 @@ class RefCopyOperation: | |||
323 | 55 | This class is just a helper to define copy operations parameters on | 56 | This class is just a helper to define copy operations parameters on |
324 | 56 | IGitHostingClient.copyRefs method. | 57 | IGitHostingClient.copyRefs method. |
325 | 57 | """ | 58 | """ |
327 | 58 | def __init__(self, source_ref, target_repo, target_ref): | 59 | def __init__(self, source_repo_path, source_ref, target_repo_path, |
328 | 60 | target_ref): | ||
329 | 61 | self.source_repo_path = source_repo_path | ||
330 | 59 | self.source_ref = source_ref | 62 | self.source_ref = source_ref |
332 | 60 | self.target_repo = target_repo | 63 | self.target_repo_path = target_repo_path |
333 | 61 | self.target_ref = target_ref | 64 | self.target_ref = target_ref |
334 | 62 | 65 | ||
335 | 63 | 66 | ||
336 | 67 | class RefDeleteOperation: | ||
337 | 68 | """A description of a ref delete operation.""" | ||
338 | 69 | def __init__(self, repo_path, ref): | ||
339 | 70 | self.repo_path = repo_path | ||
340 | 71 | self.ref = ref | ||
341 | 72 | |||
342 | 73 | |||
343 | 64 | @implementer(IGitHostingClient) | 74 | @implementer(IGitHostingClient) |
344 | 65 | class GitHostingClient: | 75 | class GitHostingClient: |
345 | 66 | """A client for the internal API provided by the Git hosting system.""" | 76 | """A client for the internal API provided by the Git hosting system.""" |
346 | @@ -277,7 +287,7 @@ class GitHostingClient: | |||
347 | 277 | json_data = { | 287 | json_data = { |
348 | 278 | "operations": [{ | 288 | "operations": [{ |
349 | 279 | "from": i.source_ref, | 289 | "from": i.source_ref, |
351 | 280 | "to": {"repo": i.target_repo, "ref": i.target_ref} | 290 | "to": {"repo": i.target_repo_path, "ref": i.target_ref} |
352 | 281 | } for i in operations] | 291 | } for i in operations] |
353 | 282 | } | 292 | } |
354 | 283 | try: | 293 | try: |
355 | @@ -299,6 +309,8 @@ class GitHostingClient: | |||
356 | 299 | 309 | ||
357 | 300 | def deleteRefs(self, refs, logger=None): | 310 | def deleteRefs(self, refs, logger=None): |
358 | 301 | """See `IGitHostingClient`.""" | 311 | """See `IGitHostingClient`.""" |
359 | 312 | # XXX pappacena: This needs to be done in a bulk operation, | ||
360 | 313 | # instead of several requests. | ||
361 | 302 | for path, ref in refs: | 314 | for path, ref in refs: |
362 | 303 | try: | 315 | try: |
363 | 304 | if logger is not None: | 316 | if logger is not None: |
364 | @@ -309,3 +321,16 @@ class GitHostingClient: | |||
365 | 309 | raise GitReferenceDeletionFault( | 321 | raise GitReferenceDeletionFault( |
366 | 310 | "Error deleting %s from repo %s: HTTP %s" % | 322 | "Error deleting %s from repo %s: HTTP %s" % |
367 | 311 | (ref, path, e.response.status_code)) | 323 | (ref, path, e.response.status_code)) |
368 | 324 | |||
369 | 325 | def bulkSyncRefs(self, copy_operations, delete_operations): | ||
370 | 326 | """See `IGitHostingClient`.""" | ||
371 | 327 | # XXX pappacena: This needs to be done in a bulk operation on Turnip | ||
372 | 328 | # side, instead of several requests. | ||
373 | 329 | operations_per_path = defaultdict(list) | ||
374 | 330 | for operation in copy_operations: | ||
375 | 331 | operations_per_path[operation.source_repo_path].append(operation) | ||
376 | 332 | for repo_path, operations in operations_per_path.items(): | ||
377 | 333 | self.copyRefs(repo_path, operations) | ||
378 | 334 | |||
379 | 335 | self.deleteRefs([(i.repo_path, i.ref) for i in delete_operations]) | ||
380 | 336 | |||
381 | diff --git a/lib/lp/code/model/gitjob.py b/lib/lp/code/model/gitjob.py | |||
382 | index ba7b9f2..4514f08 100644 | |||
383 | --- a/lib/lp/code/model/gitjob.py | |||
384 | +++ b/lib/lp/code/model/gitjob.py | |||
385 | @@ -12,6 +12,7 @@ __all__ = [ | |||
386 | 12 | 'ReclaimGitRepositorySpaceJob', | 12 | 'ReclaimGitRepositorySpaceJob', |
387 | 13 | ] | 13 | ] |
388 | 14 | 14 | ||
389 | 15 | from itertools import chain | ||
390 | 15 | 16 | ||
391 | 16 | from lazr.delegates import delegate_to | 17 | from lazr.delegates import delegate_to |
392 | 17 | from lazr.enum import ( | 18 | from lazr.enum import ( |
393 | @@ -33,7 +34,10 @@ from zope.interface import ( | |||
394 | 33 | provider, | 34 | provider, |
395 | 34 | ) | 35 | ) |
396 | 35 | 36 | ||
398 | 36 | from lp.app.enums import InformationType | 37 | from lp.app.enums import ( |
399 | 38 | InformationType, | ||
400 | 39 | PRIVATE_INFORMATION_TYPES, | ||
401 | 40 | ) | ||
402 | 37 | from lp.app.errors import NotFoundError | 41 | from lp.app.errors import NotFoundError |
403 | 38 | from lp.code.enums import ( | 42 | from lp.code.enums import ( |
404 | 39 | GitActivityType, | 43 | GitActivityType, |
405 | @@ -54,6 +58,7 @@ from lp.code.interfaces.gitjob import ( | |||
406 | 54 | ) | 58 | ) |
407 | 55 | from lp.code.interfaces.gitrule import describe_git_permissions | 59 | from lp.code.interfaces.gitrule import describe_git_permissions |
408 | 56 | from lp.code.mail.branch import BranchMailer | 60 | from lp.code.mail.branch import BranchMailer |
409 | 61 | from lp.code.model.branchmergeproposal import BranchMergeProposal | ||
410 | 57 | from lp.registry.interfaces.person import IPersonSet | 62 | from lp.registry.interfaces.person import IPersonSet |
411 | 58 | from lp.services.config import config | 63 | from lp.services.config import config |
412 | 59 | from lp.services.database.enumcol import EnumCol | 64 | from lp.services.database.enumcol import EnumCol |
413 | @@ -440,6 +445,12 @@ class GitRepositoryTransitionToInformationTypeJob(GitJobDerived): | |||
414 | 440 | def information_type(self): | 445 | def information_type(self): |
415 | 441 | return InformationType.items[self.metadata["information_type"]] | 446 | return InformationType.items[self.metadata["information_type"]] |
416 | 442 | 447 | ||
417 | 448 | def updateVirtualRefs(self): | ||
418 | 449 | merge_proposals = chain( | ||
419 | 450 | self.repository.landing_targets, | ||
420 | 451 | self.repository.landing_candidates) | ||
421 | 452 | BranchMergeProposal.bulkSyncGitVirtualRefs(merge_proposals) | ||
422 | 453 | |||
423 | 443 | def run(self): | 454 | def run(self): |
424 | 444 | """See `IGitRepositoryTransitionToInformationTypeJob`.""" | 455 | """See `IGitRepositoryTransitionToInformationTypeJob`.""" |
425 | 445 | if (self.repository.status != | 456 | if (self.repository.status != |
426 | @@ -447,7 +458,21 @@ class GitRepositoryTransitionToInformationTypeJob(GitJobDerived): | |||
427 | 447 | raise AttributeError( | 458 | raise AttributeError( |
428 | 448 | "The repository %s is not pending information type change." % | 459 | "The repository %s is not pending information type change." % |
429 | 449 | self.repository) | 460 | self.repository) |
430 | 461 | is_private = self.repository.private | ||
431 | 462 | will_be_private = self.information_type in PRIVATE_INFORMATION_TYPES | ||
432 | 450 | 463 | ||
433 | 464 | # XXX pappacena 2020-10-29: We might need to run the | ||
434 | 465 | # _transitionToInformationType as a callback (maybe on | ||
435 | 466 | # lp.code.xmlrpc.git), as a reaction to git hosting server finishing | ||
436 | 467 | # the operations to update virtual refs (call bellow). | ||
437 | 451 | self.repository._transitionToInformationType( | 468 | self.repository._transitionToInformationType( |
438 | 452 | self.information_type, self.user, self.verify_policy) | 469 | self.information_type, self.user, self.verify_policy) |
439 | 470 | |||
440 | 471 | # When changing privacy type, we need to make sure to not leak | ||
441 | 472 | # anything though virtual refs, and create new ones that didn't | ||
442 | 473 | # exist before. | ||
443 | 474 | if is_private != will_be_private: | ||
444 | 475 | # XXX pappacena 2020-10-28: We need to implement retry rules here. | ||
445 | 476 | self.updateVirtualRefs() | ||
446 | 477 | |||
447 | 453 | self.repository.status = GitRepositoryStatus.AVAILABLE | 478 | self.repository.status = GitRepositoryStatus.AVAILABLE |
448 | diff --git a/lib/lp/code/model/gitrepository.py b/lib/lp/code/model/gitrepository.py | |||
449 | index fb506f1..731ca3f 100644 | |||
450 | --- a/lib/lp/code/model/gitrepository.py | |||
451 | +++ b/lib/lp/code/model/gitrepository.py | |||
452 | @@ -1205,9 +1205,12 @@ class GitRepository(StormBase, WebhookTargetMixin, GitIdentityMixin): | |||
453 | 1205 | def updateLandingTargets(self, paths): | 1205 | def updateLandingTargets(self, paths): |
454 | 1206 | """See `IGitRepository`.""" | 1206 | """See `IGitRepository`.""" |
455 | 1207 | jobs = [] | 1207 | jobs = [] |
457 | 1208 | for merge_proposal in self.getActiveLandingTargets(paths): | 1208 | merge_proposals = self.getActiveLandingTargets(paths) |
458 | 1209 | for merge_proposal in merge_proposals: | ||
459 | 1209 | jobs.extend(merge_proposal.scheduleDiffUpdates()) | 1210 | jobs.extend(merge_proposal.scheduleDiffUpdates()) |
461 | 1210 | return jobs | 1211 | copy_ops, delete_ops = BranchMergeProposal.bulkSyncGitVirtualRefs( |
462 | 1212 | merge_proposals) | ||
463 | 1213 | return jobs, copy_ops, delete_ops | ||
464 | 1211 | 1214 | ||
465 | 1212 | def _getRecipes(self, paths=None): | 1215 | def _getRecipes(self, paths=None): |
466 | 1213 | """Undecorated version of recipes for use by `markRecipesStale`.""" | 1216 | """Undecorated version of recipes for use by `markRecipesStale`.""" |
467 | diff --git a/lib/lp/code/model/tests/test_branchmergeproposal.py b/lib/lp/code/model/tests/test_branchmergeproposal.py | |||
468 | index 81b94b3..db69320 100644 | |||
469 | --- a/lib/lp/code/model/tests/test_branchmergeproposal.py | |||
470 | +++ b/lib/lp/code/model/tests/test_branchmergeproposal.py | |||
471 | @@ -67,6 +67,10 @@ from lp.code.interfaces.branchmergeproposal import ( | |||
472 | 67 | IBranchMergeProposalGetter, | 67 | IBranchMergeProposalGetter, |
473 | 68 | IBranchMergeProposalJobSource, | 68 | IBranchMergeProposalJobSource, |
474 | 69 | ) | 69 | ) |
475 | 70 | from lp.code.interfaces.gitrepository import ( | ||
476 | 71 | GIT_CREATE_MP_VIRTUAL_REF, | ||
477 | 72 | GIT_MP_VIRTUAL_REF_FORMAT, | ||
478 | 73 | ) | ||
479 | 70 | from lp.code.model.branchmergeproposal import ( | 74 | from lp.code.model.branchmergeproposal import ( |
480 | 71 | BranchMergeProposal, | 75 | BranchMergeProposal, |
481 | 72 | BranchMergeProposalGetter, | 76 | BranchMergeProposalGetter, |
482 | @@ -97,6 +101,7 @@ from lp.testing import ( | |||
483 | 97 | ExpectedException, | 101 | ExpectedException, |
484 | 98 | launchpadlib_for, | 102 | launchpadlib_for, |
485 | 99 | login, | 103 | login, |
486 | 104 | login_admin, | ||
487 | 100 | login_person, | 105 | login_person, |
488 | 101 | person_logged_in, | 106 | person_logged_in, |
489 | 102 | TestCaseWithFactory, | 107 | TestCaseWithFactory, |
490 | @@ -256,6 +261,196 @@ class TestBranchMergeProposalPrivacy(TestCaseWithFactory): | |||
491 | 256 | self.assertContentEqual([owner, team], subscriptions) | 261 | self.assertContentEqual([owner, team], subscriptions) |
492 | 257 | 262 | ||
493 | 258 | 263 | ||
494 | 264 | class TestGitBranchMergeProposalVirtualRefs(TestCaseWithFactory): | ||
495 | 265 | """Ensure that BranchMergeProposal creation run the appropriate copy | ||
496 | 266 | and delete of virtual refs, like ref/merge/<id>/head.""" | ||
497 | 267 | |||
498 | 268 | layer = DatabaseFunctionalLayer | ||
499 | 269 | |||
500 | 270 | def setUp(self): | ||
501 | 271 | super(TestGitBranchMergeProposalVirtualRefs, self).setUp() | ||
502 | 272 | self.hosting_fixture = self.useFixture(GitHostingFixture()) | ||
503 | 273 | |||
504 | 274 | def test_should_have_vrefs_public_repos(self): | ||
505 | 275 | mp = self.factory.makeBranchMergeProposalForGit() | ||
506 | 276 | self.assertTrue(mp.should_have_git_virtual_refs) | ||
507 | 277 | |||
508 | 278 | def test_should_have_vrefs_private_source(self): | ||
509 | 279 | login_admin() | ||
510 | 280 | source_repo = self.factory.makeGitRepository( | ||
511 | 281 | information_type=InformationType.PRIVATESECURITY) | ||
512 | 282 | target_repo = self.factory.makeGitRepository(target=source_repo.target) | ||
513 | 283 | [source] = self.factory.makeGitRefs(source_repo) | ||
514 | 284 | [target] = self.factory.makeGitRefs(target_repo) | ||
515 | 285 | mp = self.factory.makeBranchMergeProposalForGit( | ||
516 | 286 | source_ref=source, target_ref=target) | ||
517 | 287 | self.assertFalse(mp.should_have_git_virtual_refs) | ||
518 | 288 | |||
519 | 289 | def test_should_have_vrefs_private_target(self): | ||
520 | 290 | login_admin() | ||
521 | 291 | source_repo = self.factory.makeGitRepository() | ||
522 | 292 | target_repo = self.factory.makeGitRepository( | ||
523 | 293 | target=source_repo.target, | ||
524 | 294 | information_type=InformationType.PRIVATESECURITY) | ||
525 | 295 | [source] = self.factory.makeGitRefs(source_repo) | ||
526 | 296 | [target] = self.factory.makeGitRefs(target_repo) | ||
527 | 297 | mp = self.factory.makeBranchMergeProposalForGit( | ||
528 | 298 | source_ref=source, target_ref=target) | ||
529 | 299 | self.assertTrue(mp.should_have_git_virtual_refs) | ||
530 | 300 | |||
531 | 301 | def test_copy_git_merge_virtual_ref(self): | ||
532 | 302 | self.useFixture(FeatureFixture({GIT_CREATE_MP_VIRTUAL_REF: "on"})) | ||
533 | 303 | mp = self.factory.makeBranchMergeProposalForGit() | ||
534 | 304 | |||
535 | 305 | copy_operations = mp.getGitRefCopyOperations() | ||
536 | 306 | self.assertEqual(1, len(copy_operations)) | ||
537 | 307 | self.assertThat(copy_operations[0], MatchesStructure( | ||
538 | 308 | source_repo_path=Equals( | ||
539 | 309 | mp.source_git_repository.getInternalPath()), | ||
540 | 310 | source_ref=Equals(mp.source_git_commit_sha1), | ||
541 | 311 | target_repo_path=Equals( | ||
542 | 312 | mp.target_git_repository.getInternalPath()), | ||
543 | 313 | target_ref=Equals("refs/merge/%s/head" % mp.id), | ||
544 | 314 | )) | ||
545 | 315 | delete_operations = mp.getGitRefDeleteOperations() | ||
546 | 316 | self.assertEqual([], delete_operations) | ||
547 | 317 | |||
548 | 318 | self.assertEqual(1, self.hosting_fixture.bulkSyncRefs.call_count) | ||
549 | 319 | args, kwargs = self.hosting_fixture.bulkSyncRefs.calls[0] | ||
550 | 320 | copy_ops, delete_ops = args | ||
551 | 321 | self.assertEqual({}, kwargs) | ||
552 | 322 | self.assertEqual([], delete_ops) | ||
553 | 323 | self.assertEqual(1, len(copy_ops)) | ||
554 | 324 | self.assertThat(copy_ops[0], MatchesStructure( | ||
555 | 325 | source_repo_path=Equals( | ||
556 | 326 | mp.source_git_repository.getInternalPath()), | ||
557 | 327 | source_ref=Equals(mp.source_git_commit_sha1), | ||
558 | 328 | target_repo_path=Equals( | ||
559 | 329 | mp.target_git_repository.getInternalPath()), | ||
560 | 330 | target_ref=Equals("refs/merge/%s/head" % mp.id), | ||
561 | 331 | )) | ||
562 | 332 | |||
563 | 333 | def test_getGitRefCopyOperations_private_source(self): | ||
564 | 334 | self.useFixture(FeatureFixture({GIT_CREATE_MP_VIRTUAL_REF: "on"})) | ||
565 | 335 | login_admin() | ||
566 | 336 | source_repo = self.factory.makeGitRepository( | ||
567 | 337 | information_type=InformationType.PRIVATESECURITY) | ||
568 | 338 | target_repo = self.factory.makeGitRepository(target=source_repo.target) | ||
569 | 339 | [source] = self.factory.makeGitRefs(source_repo) | ||
570 | 340 | [target] = self.factory.makeGitRefs(target_repo) | ||
571 | 341 | mp = self.factory.makeBranchMergeProposalForGit( | ||
572 | 342 | source_ref=source, target_ref=target) | ||
573 | 343 | self.assertEqual([], mp.getGitRefCopyOperations()) | ||
574 | 344 | |||
575 | 345 | delete_operations = mp.getGitRefDeleteOperations() | ||
576 | 346 | self.assertEqual(1, len(delete_operations)) | ||
577 | 347 | self.assertThat(delete_operations[0], MatchesStructure( | ||
578 | 348 | repo_path=Equals(mp.target_git_repository.getInternalPath()), | ||
579 | 349 | ref=Equals("refs/merge/%s/head" % mp.id), | ||
580 | 350 | )) | ||
581 | 351 | |||
582 | 352 | def test_getGitRefCopyOperations_private_source_same_repo(self): | ||
583 | 353 | self.useFixture(FeatureFixture({GIT_CREATE_MP_VIRTUAL_REF: "on"})) | ||
584 | 354 | login_admin() | ||
585 | 355 | repo = self.factory.makeGitRepository( | ||
586 | 356 | information_type=InformationType.PRIVATESECURITY) | ||
587 | 357 | [source, target] = self.factory.makeGitRefs( | ||
588 | 358 | repo, ['refs/heads/bugfix', 'refs/heads/master']) | ||
589 | 359 | mp = self.factory.makeBranchMergeProposalForGit( | ||
590 | 360 | source_ref=source, target_ref=target) | ||
591 | 361 | operations = mp.getGitRefCopyOperations() | ||
592 | 362 | self.assertEqual(1, len(operations)) | ||
593 | 363 | self.assertThat(operations[0], MatchesStructure( | ||
594 | 364 | source_repo_path=Equals( | ||
595 | 365 | mp.source_git_repository.getInternalPath()), | ||
596 | 366 | source_ref=Equals(mp.source_git_commit_sha1), | ||
597 | 367 | target_repo_path=Equals( | ||
598 | 368 | mp.target_git_repository.getInternalPath()), | ||
599 | 369 | target_ref=Equals(GIT_MP_VIRTUAL_REF_FORMAT.format(mp=mp)) | ||
600 | 370 | )) | ||
601 | 371 | |||
602 | 372 | delete_operations = mp.getGitRefDeleteOperations() | ||
603 | 373 | self.assertEqual([], delete_operations) | ||
604 | 374 | |||
605 | 375 | def test_getGitRefCopyOperations_private_source_same_owner(self): | ||
606 | 376 | self.useFixture(FeatureFixture({GIT_CREATE_MP_VIRTUAL_REF: "on"})) | ||
607 | 377 | login_admin() | ||
608 | 378 | source_repo = self.factory.makeGitRepository( | ||
609 | 379 | information_type=InformationType.PRIVATESECURITY) | ||
610 | 380 | target_repo = self.factory.makeGitRepository( | ||
611 | 381 | target=source_repo.target, owner=source_repo.owner) | ||
612 | 382 | [source] = self.factory.makeGitRefs(source_repo) | ||
613 | 383 | [target] = self.factory.makeGitRefs(target_repo) | ||
614 | 384 | mp = self.factory.makeBranchMergeProposalForGit( | ||
615 | 385 | source_ref=source, target_ref=target) | ||
616 | 386 | operations = mp.getGitRefCopyOperations() | ||
617 | 387 | self.assertEqual(1, len(operations)) | ||
618 | 388 | self.assertThat(operations[0], MatchesStructure( | ||
619 | 389 | source_repo_path=Equals( | ||
620 | 390 | mp.source_git_repository.getInternalPath()), | ||
621 | 391 | source_ref=Equals(mp.source_git_commit_sha1), | ||
622 | 392 | target_repo_path=Equals( | ||
623 | 393 | mp.target_git_repository.getInternalPath()), | ||
624 | 394 | target_ref=Equals(GIT_MP_VIRTUAL_REF_FORMAT.format(mp=mp)) | ||
625 | 395 | )) | ||
626 | 396 | |||
627 | 397 | delete_operations = mp.getGitRefDeleteOperations() | ||
628 | 398 | self.assertEqual([], delete_operations) | ||
629 | 399 | |||
630 | 400 | def test_syncGitVirtualRefs(self): | ||
631 | 401 | self.useFixture(FeatureFixture({GIT_CREATE_MP_VIRTUAL_REF: "on"})) | ||
632 | 402 | login_admin() | ||
633 | 403 | login_admin() | ||
634 | 404 | source_repo = self.factory.makeGitRepository() | ||
635 | 405 | target_repo = self.factory.makeGitRepository(target=source_repo.target) | ||
636 | 406 | [source] = self.factory.makeGitRefs(source_repo) | ||
637 | 407 | [target] = self.factory.makeGitRefs(target_repo) | ||
638 | 408 | mp = self.factory.makeBranchMergeProposalForGit( | ||
639 | 409 | source_ref=source, target_ref=target) | ||
640 | 410 | |||
641 | 411 | # mp.syncGitVirtualRefs should have been triggered by event. | ||
642 | 412 | # Check lp.code.subscribers.branchmergeproposal.merge_proposal_created. | ||
643 | 413 | self.assertEqual(1, self.hosting_fixture.bulkSyncRefs.call_count) | ||
644 | 414 | args, kwargs = self.hosting_fixture.bulkSyncRefs.calls[0] | ||
645 | 415 | copy_ops, delete_ops = args | ||
646 | 416 | self.assertEquals({}, kwargs) | ||
647 | 417 | self.assertEqual([], delete_ops) | ||
648 | 418 | self.assertEqual(1, len(copy_ops)) | ||
649 | 419 | self.assertThat(copy_ops[0], MatchesStructure( | ||
650 | 420 | source_repo_path=Equals( | ||
651 | 421 | mp.source_git_repository.getInternalPath()), | ||
652 | 422 | source_ref=Equals(mp.source_git_commit_sha1), | ||
653 | 423 | target_repo_path=Equals( | ||
654 | 424 | mp.target_git_repository.getInternalPath()), | ||
655 | 425 | target_ref=Equals("refs/merge/%s/head" % mp.id), | ||
656 | 426 | )) | ||
657 | 427 | |||
658 | 428 | self.assertEqual(0, self.hosting_fixture.deleteRefs.call_count) | ||
659 | 429 | |||
660 | 430 | def test_syncGitVirtualRefs_private_source_deletes_ref(self): | ||
661 | 431 | self.useFixture(FeatureFixture({GIT_CREATE_MP_VIRTUAL_REF: "on"})) | ||
662 | 432 | login_admin() | ||
663 | 433 | source_repo = self.factory.makeGitRepository( | ||
664 | 434 | information_type=InformationType.PRIVATESECURITY) | ||
665 | 435 | target_repo = self.factory.makeGitRepository(target=source_repo.target) | ||
666 | 436 | [source] = self.factory.makeGitRefs(source_repo) | ||
667 | 437 | [target] = self.factory.makeGitRefs(target_repo) | ||
668 | 438 | mp = self.factory.makeBranchMergeProposalForGit( | ||
669 | 439 | source_ref=source, target_ref=target) | ||
670 | 440 | |||
671 | 441 | # mp.syncGitVirtualRefs should have been triggered by event. | ||
672 | 442 | # Check lp.code.subscribers.branchmergeproposal.merge_proposal_deleted. | ||
673 | 443 | self.assertEqual(1, self.hosting_fixture.bulkSyncRefs.call_count) | ||
674 | 444 | args, kwargs = self.hosting_fixture.bulkSyncRefs.calls[0] | ||
675 | 445 | copy_ops, delete_ops = args | ||
676 | 446 | self.assertEqual({}, kwargs) | ||
677 | 447 | self.assertEqual([], copy_ops) | ||
678 | 448 | self.assertEqual(1, len(delete_ops)) | ||
679 | 449 | self.assertThat(delete_ops[0], MatchesStructure( | ||
680 | 450 | repo_path=Equals(target_repo.getInternalPath()), | ||
681 | 451 | ref=Equals("refs/merge/%s/head" % mp.id))) | ||
682 | 452 | |||
683 | 453 | |||
684 | 259 | class TestBranchMergeProposalTransitions(TestCaseWithFactory): | 454 | class TestBranchMergeProposalTransitions(TestCaseWithFactory): |
685 | 260 | """Test the state transitions of branch merge proposals.""" | 455 | """Test the state transitions of branch merge proposals.""" |
686 | 261 | 456 | ||
687 | @@ -1185,6 +1380,10 @@ class TestMergeProposalWebhooks(WithVCSScenarios, TestCaseWithFactory): | |||
688 | 1185 | def test_delete_triggers_webhooks(self): | 1380 | def test_delete_triggers_webhooks(self): |
689 | 1186 | # When an existing merge proposal is deleted, any relevant webhooks | 1381 | # When an existing merge proposal is deleted, any relevant webhooks |
690 | 1187 | # are triggered. | 1382 | # are triggered. |
691 | 1383 | self.useFixture(FeatureFixture({ | ||
692 | 1384 | GIT_CREATE_MP_VIRTUAL_REF: "on", | ||
693 | 1385 | BRANCH_MERGE_PROPOSAL_WEBHOOKS_FEATURE_FLAG: "on"})) | ||
694 | 1386 | hosting_fixture = self.useFixture(GitHostingFixture()) | ||
695 | 1188 | logger = self.useFixture(FakeLogger()) | 1387 | logger = self.useFixture(FakeLogger()) |
696 | 1189 | source = self.makeBranch() | 1388 | source = self.makeBranch() |
697 | 1190 | target = self.makeBranch(same_target_as=source) | 1389 | target = self.makeBranch(same_target_as=source) |
698 | @@ -1203,10 +1402,25 @@ class TestMergeProposalWebhooks(WithVCSScenarios, TestCaseWithFactory): | |||
699 | 1203 | expected_redacted_payload = dict( | 1402 | expected_redacted_payload = dict( |
700 | 1204 | expected_payload, | 1403 | expected_payload, |
701 | 1205 | old=MatchesDict(self.getExpectedPayload(proposal, redact=True))) | 1404 | old=MatchesDict(self.getExpectedPayload(proposal, redact=True))) |
702 | 1405 | hosting_fixture = self.useFixture(GitHostingFixture()) | ||
703 | 1206 | proposal.deleteProposal() | 1406 | proposal.deleteProposal() |
704 | 1207 | delivery = hook.deliveries.one() | 1407 | delivery = hook.deliveries.one() |
705 | 1408 | self.assertIsNotNone(delivery) | ||
706 | 1208 | self.assertCorrectDelivery(expected_payload, hook, delivery) | 1409 | self.assertCorrectDelivery(expected_payload, hook, delivery) |
707 | 1209 | self.assertCorrectLogging(expected_redacted_payload, hook, logger) | 1410 | self.assertCorrectLogging(expected_redacted_payload, hook, logger) |
708 | 1411 | if not self.git: | ||
709 | 1412 | self.assertEqual(0, hosting_fixture.bulkSyncRefs.call_count) | ||
710 | 1413 | else: | ||
711 | 1414 | self.assertEqual(1, hosting_fixture.bulkSyncRefs.call_count) | ||
712 | 1415 | args, kwargs = hosting_fixture.bulkSyncRefs.calls[0] | ||
713 | 1416 | self.assertEqual({}, kwargs) | ||
714 | 1417 | copy_ops, delete_ops = args | ||
715 | 1418 | self.assertEqual(0, len(copy_ops)) | ||
716 | 1419 | self.assertEqual(1, len(delete_ops)) | ||
717 | 1420 | self.assertThat(delete_ops[0], MatchesStructure( | ||
718 | 1421 | repo_path=Equals(target.repository.getInternalPath()), | ||
719 | 1422 | ref=Equals("refs/merge/%s/head" % proposal.id) | ||
720 | 1423 | )) | ||
721 | 1210 | 1424 | ||
722 | 1211 | 1425 | ||
723 | 1212 | class TestGetAddress(TestCaseWithFactory): | 1426 | class TestGetAddress(TestCaseWithFactory): |
724 | @@ -1507,6 +1721,26 @@ class TestBranchMergeProposalDeletion(TestCaseWithFactory): | |||
725 | 1507 | self.assertRaises( | 1721 | self.assertRaises( |
726 | 1508 | SQLObjectNotFound, BranchMergeProposalJob.get, job_id) | 1722 | SQLObjectNotFound, BranchMergeProposalJob.get, job_id) |
727 | 1509 | 1723 | ||
728 | 1724 | def test_deleteProposal_for_git_removes_virtual_ref(self): | ||
729 | 1725 | self.useFixture(FeatureFixture({GIT_CREATE_MP_VIRTUAL_REF: "on"})) | ||
730 | 1726 | self.useFixture(GitHostingFixture()) | ||
731 | 1727 | proposal = self.factory.makeBranchMergeProposalForGit() | ||
732 | 1728 | |||
733 | 1729 | # Clean up fixture calls. | ||
734 | 1730 | hosting_fixture = self.useFixture(GitHostingFixture()) | ||
735 | 1731 | proposal.deleteProposal() | ||
736 | 1732 | |||
737 | 1733 | self.assertEqual(1, hosting_fixture.bulkSyncRefs.call_count) | ||
738 | 1734 | args, kwargs = hosting_fixture.bulkSyncRefs.calls[0] | ||
739 | 1735 | self.assertEqual({}, kwargs) | ||
740 | 1736 | copy_ops, delete_ops = args | ||
741 | 1737 | self.assertEqual([], copy_ops) | ||
742 | 1738 | self.assertEqual(1, len(delete_ops)) | ||
743 | 1739 | self.assertThat(delete_ops[0], MatchesStructure( | ||
744 | 1740 | repo_path=Equals(proposal.target_git_repository.getInternalPath()), | ||
745 | 1741 | ref=Equals('refs/merge/%s/head' % proposal.id) | ||
746 | 1742 | )) | ||
747 | 1743 | |||
748 | 1510 | 1744 | ||
749 | 1511 | class TestBranchMergeProposalBugs(WithVCSScenarios, TestCaseWithFactory): | 1745 | class TestBranchMergeProposalBugs(WithVCSScenarios, TestCaseWithFactory): |
750 | 1512 | 1746 | ||
751 | diff --git a/lib/lp/code/model/tests/test_githosting.py b/lib/lp/code/model/tests/test_githosting.py | |||
752 | index 23d6063..fad0df3 100644 | |||
753 | --- a/lib/lp/code/model/tests/test_githosting.py | |||
754 | +++ b/lib/lp/code/model/tests/test_githosting.py | |||
755 | @@ -42,7 +42,6 @@ from lp.code.errors import ( | |||
756 | 42 | GitRepositoryDeletionFault, | 42 | GitRepositoryDeletionFault, |
757 | 43 | GitRepositoryScanFault, | 43 | GitRepositoryScanFault, |
758 | 44 | GitTargetError, | 44 | GitTargetError, |
759 | 45 | NoSuchGitReference, | ||
760 | 46 | ) | 45 | ) |
761 | 47 | from lp.code.interfaces.githosting import IGitHostingClient | 46 | from lp.code.interfaces.githosting import IGitHostingClient |
762 | 48 | from lp.code.model.githosting import RefCopyOperation | 47 | from lp.code.model.githosting import RefCopyOperation |
763 | @@ -414,8 +413,8 @@ class TestGitHostingClient(TestCase): | |||
764 | 414 | 413 | ||
765 | 415 | def getCopyRefOperations(self): | 414 | def getCopyRefOperations(self): |
766 | 416 | return [ | 415 | return [ |
769 | 417 | RefCopyOperation("1a2b3c4", "999", "refs/merge/123"), | 416 | RefCopyOperation("1", "1a2b3c4", "999", "refs/merge/123"), |
770 | 418 | RefCopyOperation("9a8b7c6", "666", "refs/merge/989"), | 417 | RefCopyOperation("1", "9a8b7c6", "666", "refs/merge/989"), |
771 | 419 | ] | 418 | ] |
772 | 420 | 419 | ||
773 | 421 | def test_copyRefs(self): | 420 | def test_copyRefs(self): |
774 | diff --git a/lib/lp/code/model/tests/test_gitrepository.py b/lib/lp/code/model/tests/test_gitrepository.py | |||
775 | index 08887f5..2296352 100644 | |||
776 | --- a/lib/lp/code/model/tests/test_gitrepository.py | |||
777 | +++ b/lib/lp/code/model/tests/test_gitrepository.py | |||
778 | @@ -97,6 +97,7 @@ from lp.code.interfaces.gitnamespace import ( | |||
779 | 97 | IGitNamespaceSet, | 97 | IGitNamespaceSet, |
780 | 98 | ) | 98 | ) |
781 | 99 | from lp.code.interfaces.gitrepository import ( | 99 | from lp.code.interfaces.gitrepository import ( |
782 | 100 | GIT_CREATE_MP_VIRTUAL_REF, | ||
783 | 100 | IGitRepository, | 101 | IGitRepository, |
784 | 101 | IGitRepositorySet, | 102 | IGitRepositorySet, |
785 | 102 | IGitRepositoryView, | 103 | IGitRepositoryView, |
786 | @@ -783,6 +784,7 @@ class TestGitRepositoryDeletion(TestCaseWithFactory): | |||
787 | 783 | # Make sure that the tests all flush the database changes. | 784 | # Make sure that the tests all flush the database changes. |
788 | 784 | self.addCleanup(Store.of(self.repository).flush) | 785 | self.addCleanup(Store.of(self.repository).flush) |
789 | 785 | login_person(self.user) | 786 | login_person(self.user) |
790 | 787 | self.hosting_fixture = self.useFixture(GitHostingFixture()) | ||
791 | 786 | 788 | ||
792 | 787 | def test_deletable(self): | 789 | def test_deletable(self): |
793 | 788 | # A newly created repository can be deleted without any problems. | 790 | # A newly created repository can be deleted without any problems. |
794 | @@ -824,7 +826,6 @@ class TestGitRepositoryDeletion(TestCaseWithFactory): | |||
795 | 824 | 826 | ||
796 | 825 | def test_code_import_does_not_disable_deletion(self): | 827 | def test_code_import_does_not_disable_deletion(self): |
797 | 826 | # A repository that has an attached code import can be deleted. | 828 | # A repository that has an attached code import can be deleted. |
798 | 827 | self.useFixture(GitHostingFixture()) | ||
799 | 828 | code_import = self.factory.makeCodeImport( | 829 | code_import = self.factory.makeCodeImport( |
800 | 829 | target_rcs_type=TargetRevisionControlSystems.GIT) | 830 | target_rcs_type=TargetRevisionControlSystems.GIT) |
801 | 830 | repository = code_import.git_repository | 831 | repository = code_import.git_repository |
802 | @@ -984,6 +985,7 @@ class TestGitRepositoryDeletionConsequences(TestCaseWithFactory): | |||
803 | 984 | # unsubscribe the repository owner here. | 985 | # unsubscribe the repository owner here. |
804 | 985 | self.repository.unsubscribe( | 986 | self.repository.unsubscribe( |
805 | 986 | self.repository.owner, self.repository.owner) | 987 | self.repository.owner, self.repository.owner) |
806 | 988 | self.hosting_fixture = self.useFixture(GitHostingFixture()) | ||
807 | 987 | 989 | ||
808 | 988 | def test_plain_repository(self): | 990 | def test_plain_repository(self): |
809 | 989 | # A fresh repository has no deletion requirements. | 991 | # A fresh repository has no deletion requirements. |
810 | @@ -1115,7 +1117,6 @@ class TestGitRepositoryDeletionConsequences(TestCaseWithFactory): | |||
811 | 1115 | 1117 | ||
812 | 1116 | def test_code_import_requirements(self): | 1118 | def test_code_import_requirements(self): |
813 | 1117 | # Code imports are not included explicitly in deletion requirements. | 1119 | # Code imports are not included explicitly in deletion requirements. |
814 | 1118 | self.useFixture(GitHostingFixture()) | ||
815 | 1119 | code_import = self.factory.makeCodeImport( | 1120 | code_import = self.factory.makeCodeImport( |
816 | 1120 | target_rcs_type=TargetRevisionControlSystems.GIT) | 1121 | target_rcs_type=TargetRevisionControlSystems.GIT) |
817 | 1121 | # Remove the implicit repository subscription first. | 1122 | # Remove the implicit repository subscription first. |
818 | @@ -1126,7 +1127,6 @@ class TestGitRepositoryDeletionConsequences(TestCaseWithFactory): | |||
819 | 1126 | 1127 | ||
820 | 1127 | def test_code_import_deletion(self): | 1128 | def test_code_import_deletion(self): |
821 | 1128 | # break_references allows deleting a code import repository. | 1129 | # break_references allows deleting a code import repository. |
822 | 1129 | self.useFixture(GitHostingFixture()) | ||
823 | 1130 | code_import = self.factory.makeCodeImport( | 1130 | code_import = self.factory.makeCodeImport( |
824 | 1131 | target_rcs_type=TargetRevisionControlSystems.GIT) | 1131 | target_rcs_type=TargetRevisionControlSystems.GIT) |
825 | 1132 | code_import_id = code_import.id | 1132 | code_import_id = code_import.id |
826 | @@ -1182,7 +1182,6 @@ class TestGitRepositoryDeletionConsequences(TestCaseWithFactory): | |||
827 | 1182 | 1182 | ||
828 | 1183 | def test_DeleteCodeImport(self): | 1183 | def test_DeleteCodeImport(self): |
829 | 1184 | # DeleteCodeImport.__call__ must delete the CodeImport. | 1184 | # DeleteCodeImport.__call__ must delete the CodeImport. |
830 | 1185 | self.useFixture(GitHostingFixture()) | ||
831 | 1186 | code_import = self.factory.makeCodeImport( | 1185 | code_import = self.factory.makeCodeImport( |
832 | 1187 | target_rcs_type=TargetRevisionControlSystems.GIT) | 1186 | target_rcs_type=TargetRevisionControlSystems.GIT) |
833 | 1188 | code_import_id = code_import.id | 1187 | code_import_id = code_import.id |
834 | @@ -1521,6 +1520,10 @@ class TestGitRepositoryRefs(TestCaseWithFactory): | |||
835 | 1521 | 1520 | ||
836 | 1522 | layer = DatabaseFunctionalLayer | 1521 | layer = DatabaseFunctionalLayer |
837 | 1523 | 1522 | ||
838 | 1523 | def setUp(self): | ||
839 | 1524 | super(TestGitRepositoryRefs, self).setUp() | ||
840 | 1525 | self.hosting_fixture = self.useFixture(GitHostingFixture()) | ||
841 | 1526 | |||
842 | 1524 | def test__convertRefInfo(self): | 1527 | def test__convertRefInfo(self): |
843 | 1525 | # _convertRefInfo converts a valid info dictionary. | 1528 | # _convertRefInfo converts a valid info dictionary. |
844 | 1526 | sha1 = six.ensure_text(hashlib.sha1("").hexdigest()) | 1529 | sha1 = six.ensure_text(hashlib.sha1("").hexdigest()) |
845 | @@ -1791,18 +1794,17 @@ class TestGitRepositoryRefs(TestCaseWithFactory): | |||
846 | 1791 | # planRefChanges excludes some ref prefixes by default, and can be | 1794 | # planRefChanges excludes some ref prefixes by default, and can be |
847 | 1792 | # configured otherwise. | 1795 | # configured otherwise. |
848 | 1793 | repository = self.factory.makeGitRepository() | 1796 | repository = self.factory.makeGitRepository() |
849 | 1794 | hosting_fixture = self.useFixture(GitHostingFixture()) | ||
850 | 1795 | repository.planRefChanges("dummy") | 1797 | repository.planRefChanges("dummy") |
851 | 1796 | self.assertEqual( | 1798 | self.assertEqual( |
852 | 1797 | [{"exclude_prefixes": ["refs/changes/"]}], | 1799 | [{"exclude_prefixes": ["refs/changes/"]}], |
855 | 1798 | hosting_fixture.getRefs.extract_kwargs()) | 1800 | self.hosting_fixture.getRefs.extract_kwargs()) |
856 | 1799 | hosting_fixture.getRefs.calls = [] | 1801 | self.hosting_fixture.getRefs.calls = [] |
857 | 1800 | self.pushConfig( | 1802 | self.pushConfig( |
858 | 1801 | "codehosting", git_exclude_ref_prefixes="refs/changes/ refs/pull/") | 1803 | "codehosting", git_exclude_ref_prefixes="refs/changes/ refs/pull/") |
859 | 1802 | repository.planRefChanges("dummy") | 1804 | repository.planRefChanges("dummy") |
860 | 1803 | self.assertEqual( | 1805 | self.assertEqual( |
861 | 1804 | [{"exclude_prefixes": ["refs/changes/", "refs/pull/"]}], | 1806 | [{"exclude_prefixes": ["refs/changes/", "refs/pull/"]}], |
863 | 1805 | hosting_fixture.getRefs.extract_kwargs()) | 1807 | self.hosting_fixture.getRefs.extract_kwargs()) |
864 | 1806 | 1808 | ||
865 | 1807 | def test_fetchRefCommits(self): | 1809 | def test_fetchRefCommits(self): |
866 | 1808 | # fetchRefCommits fetches detailed tip commit metadata for the | 1810 | # fetchRefCommits fetches detailed tip commit metadata for the |
867 | @@ -1875,9 +1877,8 @@ class TestGitRepositoryRefs(TestCaseWithFactory): | |||
868 | 1875 | def test_fetchRefCommits_empty(self): | 1877 | def test_fetchRefCommits_empty(self): |
869 | 1876 | # If given an empty refs dictionary, fetchRefCommits returns early | 1878 | # If given an empty refs dictionary, fetchRefCommits returns early |
870 | 1877 | # without contacting the hosting service. | 1879 | # without contacting the hosting service. |
871 | 1878 | hosting_fixture = self.useFixture(GitHostingFixture()) | ||
872 | 1879 | GitRepository.fetchRefCommits("dummy", {}) | 1880 | GitRepository.fetchRefCommits("dummy", {}) |
874 | 1880 | self.assertEqual([], hosting_fixture.getCommits.calls) | 1881 | self.assertEqual([], self.hosting_fixture.getCommits.calls) |
875 | 1881 | 1882 | ||
876 | 1882 | def test_synchroniseRefs(self): | 1883 | def test_synchroniseRefs(self): |
877 | 1883 | # synchroniseRefs copes with synchronising a repository where some | 1884 | # synchroniseRefs copes with synchronising a repository where some |
878 | @@ -1920,7 +1921,6 @@ class TestGitRepositoryRefs(TestCaseWithFactory): | |||
879 | 1920 | self.assertThat(repository.refs, MatchesSetwise(*matchers)) | 1921 | self.assertThat(repository.refs, MatchesSetwise(*matchers)) |
880 | 1921 | 1922 | ||
881 | 1922 | def test_set_default_branch(self): | 1923 | def test_set_default_branch(self): |
882 | 1923 | hosting_fixture = self.useFixture(GitHostingFixture()) | ||
883 | 1924 | repository = self.factory.makeGitRepository() | 1924 | repository = self.factory.makeGitRepository() |
884 | 1925 | self.factory.makeGitRefs( | 1925 | self.factory.makeGitRefs( |
885 | 1926 | repository=repository, | 1926 | repository=repository, |
886 | @@ -1931,22 +1931,20 @@ class TestGitRepositoryRefs(TestCaseWithFactory): | |||
887 | 1931 | self.assertEqual( | 1931 | self.assertEqual( |
888 | 1932 | [((repository.getInternalPath(),), | 1932 | [((repository.getInternalPath(),), |
889 | 1933 | {"default_branch": "refs/heads/new"})], | 1933 | {"default_branch": "refs/heads/new"})], |
891 | 1934 | hosting_fixture.setProperties.calls) | 1934 | self.hosting_fixture.setProperties.calls) |
892 | 1935 | self.assertEqual("refs/heads/new", repository.default_branch) | 1935 | self.assertEqual("refs/heads/new", repository.default_branch) |
893 | 1936 | 1936 | ||
894 | 1937 | def test_set_default_branch_unchanged(self): | 1937 | def test_set_default_branch_unchanged(self): |
895 | 1938 | hosting_fixture = self.useFixture(GitHostingFixture()) | ||
896 | 1939 | repository = self.factory.makeGitRepository() | 1938 | repository = self.factory.makeGitRepository() |
897 | 1940 | self.factory.makeGitRefs( | 1939 | self.factory.makeGitRefs( |
898 | 1941 | repository=repository, paths=["refs/heads/master"]) | 1940 | repository=repository, paths=["refs/heads/master"]) |
899 | 1942 | removeSecurityProxy(repository)._default_branch = "refs/heads/master" | 1941 | removeSecurityProxy(repository)._default_branch = "refs/heads/master" |
900 | 1943 | with person_logged_in(repository.owner): | 1942 | with person_logged_in(repository.owner): |
901 | 1944 | repository.default_branch = "master" | 1943 | repository.default_branch = "master" |
903 | 1945 | self.assertEqual([], hosting_fixture.setProperties.calls) | 1944 | self.assertEqual([], self.hosting_fixture.setProperties.calls) |
904 | 1946 | self.assertEqual("refs/heads/master", repository.default_branch) | 1945 | self.assertEqual("refs/heads/master", repository.default_branch) |
905 | 1947 | 1946 | ||
906 | 1948 | def test_set_default_branch_imported(self): | 1947 | def test_set_default_branch_imported(self): |
907 | 1949 | hosting_fixture = self.useFixture(GitHostingFixture()) | ||
908 | 1950 | repository = self.factory.makeGitRepository( | 1948 | repository = self.factory.makeGitRepository( |
909 | 1951 | repository_type=GitRepositoryType.IMPORTED) | 1949 | repository_type=GitRepositoryType.IMPORTED) |
910 | 1952 | self.factory.makeGitRefs( | 1950 | self.factory.makeGitRefs( |
911 | @@ -1959,7 +1957,7 @@ class TestGitRepositoryRefs(TestCaseWithFactory): | |||
912 | 1959 | "Cannot modify non-hosted Git repository %s." % | 1957 | "Cannot modify non-hosted Git repository %s." % |
913 | 1960 | repository.display_name, | 1958 | repository.display_name, |
914 | 1961 | setattr, repository, "default_branch", "new") | 1959 | setattr, repository, "default_branch", "new") |
916 | 1962 | self.assertEqual([], hosting_fixture.setProperties.calls) | 1960 | self.assertEqual([], self.hosting_fixture.setProperties.calls) |
917 | 1963 | self.assertEqual("refs/heads/master", repository.default_branch) | 1961 | self.assertEqual("refs/heads/master", repository.default_branch) |
918 | 1964 | 1962 | ||
919 | 1965 | def test_exception_unset_default_branch(self): | 1963 | def test_exception_unset_default_branch(self): |
920 | @@ -2581,18 +2579,66 @@ class TestGitRepositoryUpdateLandingTargets(TestCaseWithFactory): | |||
921 | 2581 | 2579 | ||
922 | 2582 | layer = DatabaseFunctionalLayer | 2580 | layer = DatabaseFunctionalLayer |
923 | 2583 | 2581 | ||
925 | 2584 | def test_schedules_diff_updates(self): | 2582 | def setUp(self): |
926 | 2583 | super(TestGitRepositoryUpdateLandingTargets, self).setUp() | ||
927 | 2584 | self.hosting_fixture = self.useFixture(GitHostingFixture()) | ||
928 | 2585 | |||
929 | 2586 | def assertSchedulesDiffUpdate(self, with_mp_virtual_ref): | ||
930 | 2585 | """Create jobs for all merge proposals.""" | 2587 | """Create jobs for all merge proposals.""" |
931 | 2586 | bmp1 = self.factory.makeBranchMergeProposalForGit() | 2588 | bmp1 = self.factory.makeBranchMergeProposalForGit() |
932 | 2587 | bmp2 = self.factory.makeBranchMergeProposalForGit( | 2589 | bmp2 = self.factory.makeBranchMergeProposalForGit( |
933 | 2588 | source_ref=bmp1.source_git_ref) | 2590 | source_ref=bmp1.source_git_ref) |
936 | 2589 | jobs = bmp1.source_git_repository.updateLandingTargets( | 2591 | |
937 | 2590 | [bmp1.source_git_path]) | 2592 | # Only enable this virtual ref here, since |
938 | 2593 | # self.factory.makeBranchMergeProposalForGit also tries to create | ||
939 | 2594 | # the virtual refs. | ||
940 | 2595 | if with_mp_virtual_ref: | ||
941 | 2596 | self.useFixture(FeatureFixture({GIT_CREATE_MP_VIRTUAL_REF: "on"})) | ||
942 | 2597 | else: | ||
943 | 2598 | self.useFixture(FeatureFixture({GIT_CREATE_MP_VIRTUAL_REF: ""})) | ||
944 | 2599 | jobs, ref_copies, ref_deletes = ( | ||
945 | 2600 | bmp1.source_git_repository.updateLandingTargets( | ||
946 | 2601 | [bmp1.source_git_path])) | ||
947 | 2591 | self.assertEqual(2, len(jobs)) | 2602 | self.assertEqual(2, len(jobs)) |
948 | 2592 | bmps_to_update = [ | 2603 | bmps_to_update = [ |
949 | 2593 | removeSecurityProxy(job).branch_merge_proposal for job in jobs] | 2604 | removeSecurityProxy(job).branch_merge_proposal for job in jobs] |
950 | 2594 | self.assertContentEqual([bmp1, bmp2], bmps_to_update) | 2605 | self.assertContentEqual([bmp1, bmp2], bmps_to_update) |
951 | 2595 | 2606 | ||
952 | 2607 | if not with_mp_virtual_ref: | ||
953 | 2608 | self.assertEqual( | ||
954 | 2609 | 0, self.hosting_fixture.bulkSyncRefs.call_count) | ||
955 | 2610 | else: | ||
956 | 2611 | self.assertEqual( | ||
957 | 2612 | 1, self.hosting_fixture.bulkSyncRefs.call_count) | ||
958 | 2613 | args, kwargs = self.hosting_fixture.bulkSyncRefs.calls[0] | ||
959 | 2614 | self.assertEqual({}, kwargs) | ||
960 | 2615 | self.assertEqual(2, len(args)) | ||
961 | 2616 | copy_ops, delete_ops = args | ||
962 | 2617 | self.assertEqual(2, len(copy_ops)) | ||
963 | 2618 | self.assertEqual(0, len(delete_ops)) | ||
964 | 2619 | self.assertThat(copy_ops[0], MatchesStructure( | ||
965 | 2620 | source_repo_path=Equals( | ||
966 | 2621 | bmp1.source_git_repository.getInternalPath()), | ||
967 | 2622 | source_ref=Equals(bmp1.source_git_commit_sha1), | ||
968 | 2623 | target_repo_path=Equals( | ||
969 | 2624 | bmp1.target_git_repository.getInternalPath()), | ||
970 | 2625 | target_ref=Equals("refs/merge/%s/head" % bmp1.id), | ||
971 | 2626 | )) | ||
972 | 2627 | self.assertThat(copy_ops[1], MatchesStructure( | ||
973 | 2628 | source_repo_path=Equals( | ||
974 | 2629 | bmp2.source_git_repository.getInternalPath()), | ||
975 | 2630 | source_ref=Equals(bmp2.source_git_commit_sha1), | ||
976 | 2631 | target_repo_path=Equals( | ||
977 | 2632 | bmp2.target_git_repository.getInternalPath()), | ||
978 | 2633 | target_ref=Equals("refs/merge/%s/head" % bmp2.id), | ||
979 | 2634 | )) | ||
980 | 2635 | |||
981 | 2636 | def test_schedules_diff_updates_with_mp_virtual_ref(self): | ||
982 | 2637 | self.assertSchedulesDiffUpdate(with_mp_virtual_ref=True) | ||
983 | 2638 | |||
984 | 2639 | def test_schedules_diff_updates_without_mp_virtual_ref(self): | ||
985 | 2640 | self.assertSchedulesDiffUpdate(with_mp_virtual_ref=False) | ||
986 | 2641 | |||
987 | 2596 | def test_ignores_final(self): | 2642 | def test_ignores_final(self): |
988 | 2597 | """Diffs for proposals in final states aren't updated.""" | 2643 | """Diffs for proposals in final states aren't updated.""" |
989 | 2598 | [source_ref] = self.factory.makeGitRefs() | 2644 | [source_ref] = self.factory.makeGitRefs() |
990 | @@ -2604,8 +2650,17 @@ class TestGitRepositoryUpdateLandingTargets(TestCaseWithFactory): | |||
991 | 2604 | for bmp in source_ref.landing_targets: | 2650 | for bmp in source_ref.landing_targets: |
992 | 2605 | if bmp.queue_status not in FINAL_STATES: | 2651 | if bmp.queue_status not in FINAL_STATES: |
993 | 2606 | removeSecurityProxy(bmp).deleteProposal() | 2652 | removeSecurityProxy(bmp).deleteProposal() |
995 | 2607 | jobs = source_ref.repository.updateLandingTargets([source_ref.path]) | 2653 | |
996 | 2654 | # Enable the feature here, since factory.makeBranchMergeProposalForGit | ||
997 | 2655 | # would also trigger the copy refs call. | ||
998 | 2656 | self.useFixture(FeatureFixture({GIT_CREATE_MP_VIRTUAL_REF: "on"})) | ||
999 | 2657 | jobs, ref_copies, ref_deletes = ( | ||
1000 | 2658 | source_ref.repository.updateLandingTargets( | ||
1001 | 2659 | [source_ref.path])) | ||
1002 | 2608 | self.assertEqual(0, len(jobs)) | 2660 | self.assertEqual(0, len(jobs)) |
1003 | 2661 | self.assertEqual(0, len(ref_copies)) | ||
1004 | 2662 | self.assertEqual(0, len(ref_deletes)) | ||
1005 | 2663 | self.assertEqual(0, self.hosting_fixture.copyRefs.call_count) | ||
1006 | 2609 | 2664 | ||
1007 | 2610 | 2665 | ||
1008 | 2611 | class TestGitRepositoryMarkRecipesStale(TestCaseWithFactory): | 2666 | class TestGitRepositoryMarkRecipesStale(TestCaseWithFactory): |
1009 | diff --git a/lib/lp/code/subscribers/branchmergeproposal.py b/lib/lp/code/subscribers/branchmergeproposal.py | |||
1010 | index a214c8f..a34ca2a 100644 | |||
1011 | --- a/lib/lp/code/subscribers/branchmergeproposal.py | |||
1012 | +++ b/lib/lp/code/subscribers/branchmergeproposal.py | |||
1013 | @@ -1,4 +1,4 @@ | |||
1015 | 1 | # Copyright 2010-2016 Canonical Ltd. This software is licensed under the | 1 | # Copyright 2010-2020 Canonical Ltd. This software is licensed under the |
1016 | 2 | # GNU Affero General Public License version 3 (see the file LICENSE). | 2 | # GNU Affero General Public License version 3 (see the file LICENSE). |
1017 | 3 | 3 | ||
1018 | 4 | """Event subscribers for branch merge proposals.""" | 4 | """Event subscribers for branch merge proposals.""" |
1019 | @@ -63,9 +63,13 @@ def _trigger_webhook(merge_proposal, payload): | |||
1020 | 63 | def merge_proposal_created(merge_proposal, event): | 63 | def merge_proposal_created(merge_proposal, event): |
1021 | 64 | """A new merge proposal has been created. | 64 | """A new merge proposal has been created. |
1022 | 65 | 65 | ||
1024 | 66 | Create a job to update the diff for the merge proposal; trigger webhooks. | 66 | Create a job to update the diff for the merge proposal; trigger webhooks |
1025 | 67 | and copy virtual refs as needed. | ||
1026 | 67 | """ | 68 | """ |
1027 | 68 | getUtility(IUpdatePreviewDiffJobSource).create(merge_proposal) | 69 | getUtility(IUpdatePreviewDiffJobSource).create(merge_proposal) |
1028 | 70 | |||
1029 | 71 | merge_proposal.syncGitVirtualRefs() | ||
1030 | 72 | |||
1031 | 69 | if getFeatureFlag(BRANCH_MERGE_PROPOSAL_WEBHOOKS_FEATURE_FLAG): | 73 | if getFeatureFlag(BRANCH_MERGE_PROPOSAL_WEBHOOKS_FEATURE_FLAG): |
1032 | 70 | payload = { | 74 | payload = { |
1033 | 71 | "action": "created", | 75 | "action": "created", |
1034 | @@ -149,3 +153,5 @@ def merge_proposal_deleted(merge_proposal, event): | |||
1035 | 149 | "old": _compose_merge_proposal_webhook_payload(merge_proposal), | 153 | "old": _compose_merge_proposal_webhook_payload(merge_proposal), |
1036 | 150 | } | 154 | } |
1037 | 151 | _trigger_webhook(merge_proposal, payload) | 155 | _trigger_webhook(merge_proposal, payload) |
1038 | 156 | |||
1039 | 157 | merge_proposal.syncGitVirtualRefs(force_delete=True) | ||
1040 | diff --git a/lib/lp/code/tests/helpers.py b/lib/lp/code/tests/helpers.py | |||
1041 | index 4ef843d..3704b06 100644 | |||
1042 | --- a/lib/lp/code/tests/helpers.py | |||
1043 | +++ b/lib/lp/code/tests/helpers.py | |||
1044 | @@ -369,6 +369,9 @@ class GitHostingFixture(fixtures.Fixture): | |||
1045 | 369 | self.getBlob = FakeMethod(result=blob) | 369 | self.getBlob = FakeMethod(result=blob) |
1046 | 370 | self.delete = FakeMethod() | 370 | self.delete = FakeMethod() |
1047 | 371 | self.disable_memcache = disable_memcache | 371 | self.disable_memcache = disable_memcache |
1048 | 372 | self.copyRefs = FakeMethod() | ||
1049 | 373 | self.deleteRefs = FakeMethod() | ||
1050 | 374 | self.bulkSyncRefs = FakeMethod() | ||
1051 | 372 | 375 | ||
1052 | 373 | def _setUp(self): | 376 | def _setUp(self): |
1053 | 374 | self.useFixture(ZopeUtilityFixture(self, IGitHostingClient)) | 377 | self.useFixture(ZopeUtilityFixture(self, IGitHostingClient)) |