launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #29437
[Merge] ~cjwatson/launchpad:git-builder-constraints into launchpad:master
Colin Watson has proposed merging ~cjwatson/launchpad:git-builder-constraints into launchpad:master.
Commit message:
Add API/UI for setting builder constraints on Git repositories
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/+git/launchpad/+merge/433671
Using a `DelimitedListWidget` for this is pretty ugly (it just gives you a text area and splits what you enter on whitespace), but it's functional, and this is only visible to admins anyway.
I believe the `launchpad.Admin` permission on repositories was previously unused, so I repurposed it for this since some builder resources will be restricted.
--
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/launchpad:git-builder-constraints into launchpad:master.
diff --git a/lib/lp/code/browser/configure.zcml b/lib/lp/code/browser/configure.zcml
index d0e3d9a..658bc83 100644
--- a/lib/lp/code/browser/configure.zcml
+++ b/lib/lp/code/browser/configure.zcml
@@ -876,6 +876,12 @@
template="../../app/templates/generic-edit.pt"/>
<browser:page
for="lp.code.interfaces.gitrepository.IGitRepository"
+ class="lp.code.browser.gitrepository.GitRepositoryEditBuilderConstraintsView"
+ permission="launchpad.Admin"
+ name="+builder-constraints"
+ template="../../app/templates/generic-edit.pt"/>
+ <browser:page
+ for="lp.code.interfaces.gitrepository.IGitRepository"
class="lp.code.browser.gitrepository.GitRepositoryEditView"
permission="launchpad.Edit"
name="+edit"
diff --git a/lib/lp/code/browser/gitrepository.py b/lib/lp/code/browser/gitrepository.py
index da7dc76..bc941d7 100644
--- a/lib/lp/code/browser/gitrepository.py
+++ b/lib/lp/code/browser/gitrepository.py
@@ -55,6 +55,7 @@ from lp.app.browser.launchpadform import (
from lp.app.errors import NotFoundError, UnexpectedFormData
from lp.app.vocabularies import InformationTypeVocabulary
from lp.app.widgets.itemswidgets import LaunchpadRadioWidgetWithDescription
+from lp.app.widgets.textwidgets import DelimitedListWidget
from lp.charms.browser.hascharmrecipes import HasCharmRecipesViewMixin
from lp.code.browser.branch import CodeEditOwnerMixin
from lp.code.browser.branchmergeproposal import (
@@ -270,6 +271,7 @@ class GitRepositoryEditMenu(NavigationMenu):
"activity",
"access_tokens",
"webhooks",
+ "builder_constraints",
"delete",
]
@@ -303,6 +305,11 @@ class GitRepositoryEditMenu(NavigationMenu):
text = "Manage webhooks"
return Link("+webhooks", text, icon="edit")
+ @enabled_with_permission("launchpad.Admin")
+ def builder_constraints(self):
+ text = "Set builder constraints"
+ return Link("+builder-constraints", text, icon="edit")
+
@enabled_with_permission("launchpad.Edit")
def delete(self):
text = "Delete repository"
@@ -626,7 +633,10 @@ class GitRepositoryEditFormView(LaunchpadEditFormView):
normally editable through the interface.
"""
- use_template(IGitRepository, include=["default_branch"])
+ use_template(
+ IGitRepository,
+ include=["builder_constraints", "default_branch"],
+ )
information_type = copy_field(
IGitRepository["information_type"],
readonly=False,
@@ -776,6 +786,15 @@ class GitRepositoryEditReviewerView(GitRepositoryEditFormView):
return {"reviewer": self.context.code_reviewer}
+class GitRepositoryEditBuilderConstraintsView(GitRepositoryEditFormView):
+ """A view to set builder constraints."""
+
+ field_names = ["builder_constraints"]
+ custom_widget_builder_constraints = CustomWidgetFactory(
+ DelimitedListWidget, height=5
+ )
+
+
class GitRepositoryEditView(CodeEditOwnerMixin, GitRepositoryEditFormView):
"""The main view for editing repository attributes."""
diff --git a/lib/lp/code/browser/tests/test_gitrepository.py b/lib/lp/code/browser/tests/test_gitrepository.py
index 9261c4b..23b7891 100644
--- a/lib/lp/code/browser/tests/test_gitrepository.py
+++ b/lib/lp/code/browser/tests/test_gitrepository.py
@@ -33,6 +33,7 @@ from zope.formlib.itemswidgets import ItemDisplayWidget
from zope.publisher.interfaces import NotFound
from zope.security.interfaces import Unauthorized
from zope.security.proxy import removeSecurityProxy
+from zope.testbrowser.browser import LinkNotFoundError
from lp.app.enums import InformationType
from lp.app.errors import UnexpectedFormData
@@ -71,6 +72,7 @@ from lp.testing import (
StormStatementRecorder,
TestCaseWithFactory,
admin_logged_in,
+ login_celebrity,
login_person,
logout,
person_logged_in,
@@ -1070,6 +1072,42 @@ class TestGitRepositoryEditReviewerView(TestCaseWithFactory):
self.assertEqual(modified_date, repository.date_last_modified)
+class TestGitRepositoryEditBuilderConstraintsView(BrowserTestCase):
+
+ layer = DatabaseFunctionalLayer
+
+ def test_unauthorized(self):
+ # A non-commercial-admin user cannot administer a Git repository.
+ self.useFixture(FakeLogger())
+ owner = self.factory.makePerson()
+ repository = self.factory.makeGitRepository(owner=owner)
+ repository_url = canonical_url(repository, rootsite="code")
+ browser = self.getViewBrowser(repository, user=owner)
+ self.assertRaises(
+ LinkNotFoundError, browser.getLink, "Set builder constraints"
+ )
+ self.assertRaises(
+ Unauthorized,
+ self.getUserBrowser,
+ repository_url + "/+builder-constraints",
+ user=owner,
+ )
+
+ def test_commercial_admin(self):
+ # A commercial admin can set builder constraints on a Git
+ # repository.
+ owner = self.factory.makePerson()
+ repository = self.factory.makeGitRepository(owner=owner)
+ commercial_admin = login_celebrity("commercial_admin")
+ browser = self.getViewBrowser(repository, user=commercial_admin)
+ browser.getLink("Set builder constraints").click()
+ browser.getControl("Builder constraints").value = "gpu\nlarge"
+ browser.getControl("Change Git Repository").click()
+
+ login_person(owner)
+ self.assertEqual(["gpu", "large"], repository.builder_constraints)
+
+
class TestGitRepositoryEditView(TestCaseWithFactory):
layer = DatabaseFunctionalLayer
diff --git a/lib/lp/code/configure.zcml b/lib/lp/code/configure.zcml
index f28603c..035facb 100644
--- a/lib/lp/code/configure.zcml
+++ b/lib/lp/code/configure.zcml
@@ -828,7 +828,8 @@
interface="lp.app.interfaces.launchpad.IPrivacy
lp.code.interfaces.gitrepository.IGitRepositoryView
lp.code.interfaces.gitrepository.IGitRepositoryModerateAttributes
- lp.code.interfaces.gitrepository.IGitRepositoryEditableAttributes" />
+ lp.code.interfaces.gitrepository.IGitRepositoryEditableAttributes
+ lp.code.interfaces.gitrepository.IGitRepositoryAdminAttributes" />
<require
permission="launchpad.Moderate"
interface="lp.code.interfaces.gitrepository.IGitRepositoryModerate"
@@ -841,6 +842,9 @@
<require
permission="launchpad.ExpensiveRequest"
interface="lp.code.interfaces.gitrepository.IGitRepositoryExpensiveRequest"/>
+ <require
+ permission="launchpad.Admin"
+ set_schema="lp.code.interfaces.gitrepository.IGitRepositoryAdminAttributes" />
</class>
<adapter
for="lp.code.interfaces.gitrepository.IGitRepository"
diff --git a/lib/lp/code/interfaces/gitrepository.py b/lib/lp/code/interfaces/gitrepository.py
index f78cd09..93c8553 100644
--- a/lib/lp/code/interfaces/gitrepository.py
+++ b/lib/lp/code/interfaces/gitrepository.py
@@ -307,16 +307,6 @@ class IGitRepositoryView(IHasRecipes):
)
)
- builder_constraints = List(
- title=_("Builder constraints"),
- required=False,
- readonly=True,
- value_type=TextLine(),
- description=_(
- "Builder resource tags required by builds of this repository."
- ),
- )
-
def getClonedFrom():
"""Returns from which repository the given repo is a clone from."""
@@ -1250,6 +1240,26 @@ class IGitRepositoryEdit(IWebhookTarget, IAccessTokenTarget):
"""
+class IGitRepositoryAdminAttributes(Interface):
+ """`IGitRepository` attributes that can be edited by admins.
+
+ These attributes need launchpad.View to see, and launchpad.Admin to change.
+ """
+
+ builder_constraints = exported(
+ List(
+ title=_("Builder constraints"),
+ required=False,
+ readonly=False,
+ value_type=TextLine(),
+ description=_(
+ "Builder resource tags required by builds of this repository."
+ ),
+ ),
+ as_of="devel",
+ )
+
+
# XXX cjwatson 2015-01-19 bug=760849: "beta" is a lie to get WADL
# generation working. Individual attributes must set their version to
# "devel".
@@ -1261,6 +1271,7 @@ class IGitRepository(
IGitRepositoryEditableAttributes,
IGitRepositoryEdit,
IGitRepositoryExpensiveRequest,
+ IGitRepositoryAdminAttributes,
):
"""A Git repository."""
diff --git a/lib/lp/code/model/tests/test_gitrepository.py b/lib/lp/code/model/tests/test_gitrepository.py
index 0363da5..ec2b04b 100644
--- a/lib/lp/code/model/tests/test_gitrepository.py
+++ b/lib/lp/code/model/tests/test_gitrepository.py
@@ -6601,6 +6601,46 @@ class TestGitRepositoryWebservice(TestCaseWithFactory):
response.body,
)
+ def test_builder_constraints_commercial_admin(self):
+ # A commercial admin can change a repository's builder constraints.
+ repository_db = self.factory.makeGitRepository()
+ commercial_admin = getUtility(
+ ILaunchpadCelebrities
+ ).commercial_admin.teamowner
+ webservice = webservice_for_person(
+ commercial_admin, permission=OAuthPermission.WRITE_PUBLIC
+ )
+ webservice.default_api_version = "devel"
+ with person_logged_in(ANONYMOUS):
+ repository_url = api_url(repository_db)
+ response = webservice.patch(
+ repository_url,
+ "application/json",
+ json.dumps({"builder_constraints": ["gpu"]}),
+ )
+ self.assertEqual(209, response.status)
+ with person_logged_in(ANONYMOUS):
+ self.assertEqual(["gpu"], repository_db.builder_constraints)
+
+ def test_builder_constraints_owner(self):
+ # The owner of a repository cannot change its builder constraints
+ # (unless they're also a (commercial) admin).
+ repository_db = self.factory.makeGitRepository()
+ webservice = webservice_for_person(
+ repository_db.owner, permission=OAuthPermission.WRITE_PUBLIC
+ )
+ webservice.default_api_version = "devel"
+ with person_logged_in(ANONYMOUS):
+ repository_url = api_url(repository_db)
+ response = webservice.patch(
+ repository_url,
+ "application/json",
+ json.dumps({"builder_constraints": ["gpu"]}),
+ )
+ self.assertEqual(401, response.status)
+ with person_logged_in(ANONYMOUS):
+ self.assertIsNone(repository_db.builder_constraints)
+
class TestGitRepositoryMacaroonIssuer(MacaroonTestMixin, TestCaseWithFactory):
"""Test GitRepository macaroon issuing and verification."""
diff --git a/lib/lp/code/security.py b/lib/lp/code/security.py
index d6852ed..3a85550 100644
--- a/lib/lp/code/security.py
+++ b/lib/lp/code/security.py
@@ -50,8 +50,8 @@ from lp.registry.interfaces.distributionsourcepackage import (
from lp.registry.interfaces.ociproject import IOCIProject
from lp.registry.interfaces.product import IProduct
from lp.security import (
- AdminByAdminsTeam,
AdminByBuilddAdmin,
+ AdminByCommercialTeamOrAdmins,
OnlyBazaarExpertsAndAdmins,
OnlyVcsImportsAndAdmins,
)
@@ -296,9 +296,14 @@ class ModerateGitRepository(EditGitRepository):
return user.in_commercial_admin
-class AdminGitRepository(AdminByAdminsTeam):
- """The admins can administer Git repositories."""
+class AdminGitRepository(AdminByCommercialTeamOrAdmins):
+ """Restrict changing builder constraints on Git repositories.
+ The security of some parts of the build farm depends on these settings,
+ so they can only be changed by (commercial) admins.
+ """
+
+ permission = "launchpad.Admin"
usedfor = IGitRepository
