launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #25218
[Merge] ~cjwatson/launchpad:oci-new-push-rule-credentials-owner into launchpad:master
Colin Watson has proposed merging ~cjwatson/launchpad:oci-new-push-rule-credentials-owner into launchpad:master.
Commit message:
Export OCIRecipe.newPushRule(credentials_owner) on the API
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/+git/launchpad/+merge/389760
It normally makes sense to allow controlling the owner of newly-created objects such as OCI registry credentials in webservice API methods. It's also more likely for the recipe owner to be a reasonable default for the owner of new credentials than the logged-in user: team-owned recipes will normally want team-owned credentials too.
--
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/launchpad:oci-new-push-rule-credentials-owner into launchpad:master.
diff --git a/lib/lp/oci/interfaces/ocirecipe.py b/lib/lp/oci/interfaces/ocirecipe.py
index 37f74a2..c433825 100644
--- a/lib/lp/oci/interfaces/ocirecipe.py
+++ b/lib/lp/oci/interfaces/ocirecipe.py
@@ -300,7 +300,6 @@ class IOCIRecipeEdit(IWebhookTarget):
def destroySelf():
"""Delete this OCI recipe, provided that it has no builds."""
- @call_with(owner=REQUEST_USER)
@operation_parameters(
registry_url=TextLine(
title=_("Registry URL"),
@@ -314,11 +313,16 @@ class IOCIRecipeEdit(IWebhookTarget):
title=_("Registry credentials"),
description=_(
"The credentials to use in pushing the image to the registry"),
- required=True))
+ required=True),
+ credentials_owner=PersonChoice(
+ title=_("Registry credentials owner"),
+ required=False,
+ vocabulary="AllUserTeamsParticipationPlusSelf"))
# Really IOCIPushRule, patched in lp.oci.interfaces.webservice.
@export_factory_operation(Interface, [])
@operation_for_version("devel")
- def newPushRule(owner, registry_url, image_name, credentials):
+ def newPushRule(registry_url, image_name, credentials,
+ credentials_owner=None):
"""Add a new rule for pushing builds of this recipe to a registry."""
diff --git a/lib/lp/oci/model/ocirecipe.py b/lib/lp/oci/model/ocirecipe.py
index 0125114..2a83ccc 100644
--- a/lib/lp/oci/model/ocirecipe.py
+++ b/lib/lp/oci/model/ocirecipe.py
@@ -498,11 +498,17 @@ class OCIRecipe(Storm, WebhookTargetMixin):
def can_upload_to_registry(self):
return not self.push_rules.is_empty()
- def newPushRule(self, owner, registry_url, image_name, credentials):
+ def newPushRule(self, registry_url, image_name, credentials,
+ credentials_owner=None):
"""See `IOCIRecipe`."""
-
+ if credentials_owner is None:
+ # Ideally this should probably be a required parameter, but
+ # earlier versions of this method didn't allow passing the
+ # credentials owner via the webservice API, so for compatibility
+ # we give it a default.
+ credentials_owner = self.owner
oci_credentials = getUtility(IOCIRegistryCredentialsSet).getOrCreate(
- owner, registry_url, credentials)
+ credentials_owner, registry_url, credentials)
push_rule = getUtility(IOCIPushRuleSet).new(
self, oci_credentials, image_name)
Store.of(push_rule).flush()
diff --git a/lib/lp/oci/tests/helpers.py b/lib/lp/oci/tests/helpers.py
index e199241..52561a3 100644
--- a/lib/lp/oci/tests/helpers.py
+++ b/lib/lp/oci/tests/helpers.py
@@ -11,6 +11,11 @@ __all__ = []
import base64
from nacl.public import PrivateKey
+from testtools.matchers import (
+ AfterPreprocessing,
+ MatchesAll,
+ )
+from zope.security.proxy import removeSecurityProxy
from lp.oci.interfaces.ocirecipe import OCI_RECIPE_ALLOW_CREATE
from lp.services.features.testing import FeatureFixture
@@ -30,3 +35,18 @@ class OCIConfigHelperMixin:
bytes(self.private_key)).decode("UTF-8"))
# Default feature flags for our tests
self.useFixture(FeatureFixture({OCI_RECIPE_ALLOW_CREATE: 'on'}))
+
+
+class MatchesOCIRegistryCredentials(MatchesAll):
+ """Matches an `OCIRegistryCredentials` object.
+
+ `main_matcher` matches the `OCIRegistryCredentials` object itself, while
+ `credentials_matcher` matches the result of its `getCredentials` method.
+ """
+
+ def __init__(self, main_matcher, credentials_matcher):
+ super(MatchesOCIRegistryCredentials, self).__init__(
+ main_matcher,
+ AfterPreprocessing(
+ lambda matchee: removeSecurityProxy(matchee).getCredentials(),
+ credentials_matcher))
diff --git a/lib/lp/oci/tests/test_ocirecipe.py b/lib/lp/oci/tests/test_ocirecipe.py
index 880c351..ad67a7a 100644
--- a/lib/lp/oci/tests/test_ocirecipe.py
+++ b/lib/lp/oci/tests/test_ocirecipe.py
@@ -8,10 +8,7 @@ from __future__ import absolute_import, print_function, unicode_literals
import json
from fixtures import FakeLogger
-from six import (
- ensure_text,
- string_types,
- )
+from six import string_types
from storm.exceptions import LostObjectError
from storm.store import Store
from testtools.matchers import (
@@ -51,7 +48,10 @@ from lp.oci.interfaces.ocirecipe import (
)
from lp.oci.interfaces.ocirecipebuild import IOCIRecipeBuildSet
from lp.oci.interfaces.ocirecipejob import IOCIRecipeRequestBuildsJobSource
-from lp.oci.tests.helpers import OCIConfigHelperMixin
+from lp.oci.tests.helpers import (
+ MatchesOCIRegistryCredentials,
+ OCIConfigHelperMixin,
+ )
from lp.registry.interfaces.series import SeriesStatus
from lp.services.config import config
from lp.services.database.constants import (
@@ -453,39 +453,53 @@ class TestOCIRecipe(OCIConfigHelperMixin, TestCaseWithFactory):
self.setConfig()
recipe = self.factory.makeOCIRecipe()
url = self.factory.getUniqueURL()
- image_name = ensure_text(self.factory.getUniqueString())
+ image_name = self.factory.getUniqueUnicode()
credentials = {
"username": "test-username", "password": "test-password"}
- with person_logged_in(recipe.owner):
+ with person_logged_in(recipe.registrant):
push_rule = recipe.newPushRule(
- recipe.owner, url, image_name, credentials)
- self.assertEqual(
- image_name,
- push_rule.image_name)
- self.assertEqual(
- url,
- push_rule.registry_credentials.url)
- self.assertEqual(
- credentials,
- push_rule.registry_credentials.getCredentials())
-
- self.assertEqual(
- push_rule,
- recipe.push_rules[0])
+ url, image_name, credentials,
+ credentials_owner=recipe.registrant)
+ self.assertThat(push_rule, MatchesStructure(
+ image_name=Equals(image_name),
+ registry_credentials=MatchesOCIRegistryCredentials(
+ MatchesStructure.byEquality(
+ owner=recipe.registrant, url=url),
+ Equals(credentials))))
+ self.assertEqual(push_rule, recipe.push_rules[0])
+
+ def test_newPushRule_default_owner(self):
+ # The registry credentials for a new push rule default to being
+ # owned by the recipe owner.
+ self.setConfig()
+ recipe = self.factory.makeOCIRecipe()
+ url = self.factory.getUniqueURL()
+ image_name = self.factory.getUniqueUnicode()
+ credentials = {
+ "username": "test-username", "password": "test-password"}
+
+ with person_logged_in(recipe.registrant):
+ push_rule = recipe.newPushRule(url, image_name, credentials)
+ self.assertThat(push_rule, MatchesStructure(
+ image_name=Equals(image_name),
+ registry_credentials=MatchesOCIRegistryCredentials(
+ MatchesStructure.byEquality(owner=recipe.owner, url=url),
+ Equals(credentials))))
+ self.assertEqual(push_rule, recipe.push_rules[0])
def test_newPushRule_invalid_url(self):
self.setConfig()
recipe = self.factory.makeOCIRecipe()
url = 'asdf://foo.com'
- image_name = ensure_text(self.factory.getUniqueString())
+ image_name = self.factory.getUniqueUnicode()
credentials = {
"username": "test-username", "password": "test-password"}
with person_logged_in(recipe.owner):
self.assertRaises(
ValidationError, recipe.newPushRule,
- recipe.owner, url, image_name, credentials)
+ url, image_name, credentials, credentials_owner=recipe.owner)
# Avoid trying to flush the incomplete object on cleanUp.
Store.of(recipe).rollback()
@@ -493,17 +507,17 @@ class TestOCIRecipe(OCIConfigHelperMixin, TestCaseWithFactory):
self.setConfig()
recipe = self.factory.makeOCIRecipe()
url = self.factory.getUniqueURL()
- image_name = ensure_text(self.factory.getUniqueString())
+ image_name = self.factory.getUniqueUnicode()
credentials = {
"username": "test-username", "password": "test-password"}
with person_logged_in(recipe.owner):
recipe.newPushRule(
- recipe.owner, url, image_name, credentials)
+ url, image_name, credentials, credentials_owner=recipe.owner)
self.assertRaises(
OCIPushRuleAlreadyExists,
recipe.newPushRule,
- recipe.owner, url, image_name, credentials)
+ url, image_name, credentials, credentials_owner=recipe.owner)
def test_set_official_directly_is_forbidden(self):
recipe = self.factory.makeOCIRecipe()
@@ -1151,7 +1165,11 @@ class TestOCIRecipeWebservice(OCIConfigHelperMixin, TestCaseWithFactory):
new_obj_url = resp.getHeader("Location")
ws_push_rule = self.load_from_api(new_obj_url)
- self.assertEqual(obj["image_name"], ws_push_rule["image_name"])
+ self.assertThat(ws_push_rule, ContainsDict({
+ "image_name": Equals(obj["image_name"]),
+ "registry_url": Equals(obj["registry_url"]),
+ "username": Equals("foo"),
+ }))
def test_api_push_rules_exported(self):
"""Are push rules exported for a recipe?"""
diff --git a/lib/lp/registry/browser/tests/test_person.py b/lib/lp/registry/browser/tests/test_person.py
index 029f12c..e3f74db 100644
--- a/lib/lp/registry/browser/tests/test_person.py
+++ b/lib/lp/registry/browser/tests/test_person.py
@@ -22,11 +22,9 @@ from testscenarios import (
WithScenarios,
)
from testtools.matchers import (
- AfterPreprocessing,
DocTestMatches,
Equals,
LessThan,
- MatchesAll,
MatchesDict,
MatchesSetwise,
MatchesStructure,
@@ -50,7 +48,10 @@ from lp.oci.interfaces.ocirecipe import OCI_RECIPE_ALLOW_CREATE
from lp.oci.interfaces.ociregistrycredentials import (
IOCIRegistryCredentialsSet,
)
-from lp.oci.tests.helpers import OCIConfigHelperMixin
+from lp.oci.tests.helpers import (
+ MatchesOCIRegistryCredentials,
+ OCIConfigHelperMixin,
+ )
from lp.registry.browser.person import PersonView
from lp.registry.browser.team import TeamInvitationView
from lp.registry.enums import PersonVisibility
@@ -1322,21 +1323,6 @@ class TestPersonRelatedProjectsView(TestCaseWithFactory):
self.assertThat(view(), next_match)
-class MatchesOCIRegistryCredentials(MatchesAll):
- """Matches an `OCIRegistryCredentials` object.
-
- `main_matcher` matches the `OCIRegistryCredentials` object itself, while
- `credentials_matcher` matches the result of its `getCredentials` method.
- """
-
- def __init__(self, main_matcher, credentials_matcher):
- super(MatchesOCIRegistryCredentials, self).__init__(
- main_matcher,
- AfterPreprocessing(
- lambda matchee: removeSecurityProxy(matchee).getCredentials(),
- credentials_matcher))
-
-
class TestPersonOCIRegistryCredentialsView(
WithScenarios, BrowserTestCase, OCIConfigHelperMixin):