launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #26145
[Merge] ~twom/launchpad:oci-policy-git-branch-picker-format into launchpad:master
Tom Wardill has proposed merging ~twom/launchpad:oci-policy-git-branch-picker-format into launchpad:master.
Commit message:
Add validator to OCIRecipeAdd and OCIRecipeEdit views
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~twom/launchpad/+git/launchpad/+merge/397175
Ensure that any new OCIRecipes, and any that are updated, use the branch format of `2.0-20.04`.
--
Your team Launchpad code reviewers is requested to review the proposed merge of ~twom/launchpad:oci-policy-git-branch-picker-format into launchpad:master.
diff --git a/lib/lp/app/validators/validation.py b/lib/lp/app/validators/validation.py
index e38af13..e9ef8b1 100644
--- a/lib/lp/app/validators/validation.py
+++ b/lib/lp/app/validators/validation.py
@@ -9,8 +9,11 @@ __all__ = [
'can_be_nominated_for_series',
'valid_cve_sequence',
'validate_new_team_email',
+ 'validate_oci_branch_name',
]
+import re
+
from zope.component import getUtility
from lp import _
@@ -79,3 +82,25 @@ def validate_new_team_email(email):
_validate_email(email)
_check_email_availability(email)
return True
+
+
+def validate_oci_branch_name(branch_name):
+ """Check that a git ref name matches appversion-ubuntuversion."""
+ split = branch_name.split('-')
+ # if we've not got at least two components
+ if len(split) < 2:
+ return False
+ app_version = split[0:-1]
+ ubuntu_version = split[-1]
+ # 20.04 format
+ ubuntu_match = re.match("\d{2}\.\d{2}", ubuntu_version)
+ if not ubuntu_match:
+ return False
+ # disallow risks in app version number
+ for risk in ["stable", "candidate", "beta", "edge"]:
+ if risk in app_version:
+ return False
+ # no '/' as they're a delimiter
+ if '/' in app_version:
+ return False
+ return True
diff --git a/lib/lp/code/browser/widgets/gitref.py b/lib/lp/code/browser/widgets/gitref.py
index cbe203d..43c3a03 100644
--- a/lib/lp/code/browser/widgets/gitref.py
+++ b/lib/lp/code/browser/widgets/gitref.py
@@ -107,6 +107,8 @@ class GitRefWidget(BrowserWidget, InputWidget):
# If True, only allow reference paths to be branches (refs/heads/*).
require_branch = False
+ branch_validator = None
+
def setUpSubWidgets(self):
if self._widgets_set_up:
return
@@ -126,6 +128,9 @@ class GitRefWidget(BrowserWidget, InputWidget):
self, field.__name__, field, IInputWidget, prefix=self.name)
self._widgets_set_up = True
+ def setBranchFormatValidator(self, branch_validator):
+ self.branch_validator = branch_validator
+
def setRenderedValue(self, value, with_path=True):
"""See `IWidget`."""
self.setUpSubWidgets()
@@ -203,11 +208,19 @@ class GitRefWidget(BrowserWidget, InputWidget):
self.path_widget._getFormInput())))
else:
ref = None
+ import ipdb; ipdb.set_trace()
if not ref and (repository or self.context.required):
raise WidgetInputError(
+ self.name, self.label,
+ LaunchpadValidationError(
+ "Please enter a Git branch path."))
+ if self.branch_validator and ref is not None:
+ valid, message = self.branch_validator(ref)
+ if not valid:
+ raise WidgetInputError(
self.name, self.label,
LaunchpadValidationError(
- "Please enter a Git branch path."))
+ message))
return ref
def error(self):
diff --git a/lib/lp/code/browser/widgets/tests/test_gitrefwidget.py b/lib/lp/code/browser/widgets/tests/test_gitrefwidget.py
index 98e223a..9b8e76c 100644
--- a/lib/lp/code/browser/widgets/tests/test_gitrefwidget.py
+++ b/lib/lp/code/browser/widgets/tests/test_gitrefwidget.py
@@ -274,6 +274,32 @@ class TestGitRefWidget(WithScenarios, TestCaseWithFactory):
self.widget.request = LaunchpadTestRequest(form=form)
self.assertEqual(ref, self.widget.getInputValue())
+ def test_getInputValue_with_branch_validator_valid(self):
+ def validator(ref):
+ return True, ""
+ [ref] = self.factory.makeGitRefs()
+ form = {
+ "field.git_ref.repository": ref.repository.unique_name,
+ "field.git_ref.path": ref.path,
+ }
+ self.widget.setBranchFormatValidator(validator)
+ self.widget.request = LaunchpadTestRequest(form=form)
+ self.assertEqual(ref, self.widget.getInputValue())
+
+ def test_getInputValue_with_branch_validator_invalid(self):
+ def validator(ref):
+ return False, "Not in correct format"
+ [ref] = self.factory.makeGitRefs()
+ form = {
+ "field.git_ref.repository": ref.repository.unique_name,
+ "field.git_ref.path": ref.path,
+ }
+ self.widget.setBranchFormatValidator(validator)
+ self.widget.request = LaunchpadTestRequest(form=form)
+ self.assertGetInputValueError(
+ form,
+ "Not in correct format")
+
def test_call(self):
# The __call__ method sets up the widgets.
markup = self.widget()
diff --git a/lib/lp/oci/browser/ocirecipe.py b/lib/lp/oci/browser/ocirecipe.py
index abaff05..4f4253a 100644
--- a/lib/lp/oci/browser/ocirecipe.py
+++ b/lib/lp/oci/browser/ocirecipe.py
@@ -54,6 +54,7 @@ from lp.app.browser.tales import (
GitRepositoryFormatterAPI,
)
from lp.app.errors import UnexpectedFormData
+from lp.app.validators.validation import validate_oci_branch_name
from lp.app.widgets.itemswidgets import LabeledMultiCheckBoxWidget
from lp.buildmaster.interfaces.processor import IProcessorSet
from lp.code.browser.widgets.gitref import GitRefWidget
@@ -754,6 +755,16 @@ class OCIRecipeFormMixin:
return True
return False
+ def _branch_format_validator(self, ref):
+ result = validate_oci_branch_name(ref.name)
+ if result:
+ return result, ""
+ message = (
+ "Branch does not match format "
+ "'applicationversion-ubuntuversion', eg. 'v1.0-20.04'"
+ )
+ return False, message
+
class OCIRecipeAddView(LaunchpadFormView, EnableProcessorsMixin,
OCIRecipeFormMixin):
@@ -805,6 +816,7 @@ class OCIRecipeAddView(LaunchpadFormView, EnableProcessorsMixin,
path = self.context.getDefaultGitRepositoryPath(self.user)
widget = self.widgets["git_ref"]
widget.setUpSubWidgets()
+ widget.setBranchFormatValidator(self._branch_format_validator)
widget.repository_widget.setRenderedValue(path)
if widget.error():
# Do not override more important git_ref errors.
@@ -950,6 +962,7 @@ class OCIRecipeEditView(BaseOCIRecipeEditView, EnableProcessorsMixin,
oci_proj_url = canonical_url(oci_proj)
widget = self.widgets["git_ref"]
widget.setUpSubWidgets()
+ widget.setBranchFormatValidator(self._branch_format_validator)
if widget.error():
# Do not override more important git_ref errors.
return
diff --git a/lib/lp/oci/browser/tests/test_ocirecipe.py b/lib/lp/oci/browser/tests/test_ocirecipe.py
index 22634c1..970c49c 100644
--- a/lib/lp/oci/browser/tests/test_ocirecipe.py
+++ b/lib/lp/oci/browser/tests/test_ocirecipe.py
@@ -190,7 +190,8 @@ class TestOCIRecipeAddView(BaseTestOCIRecipeView):
def test_create_new_recipe(self):
oci_project = self.factory.makeOCIProject()
oci_project_display = oci_project.display_name
- [git_ref] = self.factory.makeGitRefs()
+ [git_ref] = self.factory.makeGitRefs(
+ paths=['/refs/heads/v2.0-20.04'])
source_display = git_ref.display_name
browser = self.getViewBrowser(
oci_project, view_name="+new-recipe", user=self.person)
@@ -227,9 +228,24 @@ class TestOCIRecipeAddView(BaseTestOCIRecipeView):
"Official recipe:\nNo",
MatchesTagText(content, "official-recipe"))
+ def test_create_new_recipe_invalid_format(self):
+ oci_project = self.factory.makeOCIProject()
+ [git_ref] = self.factory.makeGitRefs(
+ paths=['/refs/heads/invalid'])
+ browser = self.getViewBrowser(
+ oci_project, view_name="+new-recipe", user=self.person)
+ browser.getControl(name="field.name").value = "recipe-name"
+ browser.getControl("Description").value = "Recipe description"
+ browser.getControl(name="field.git_ref.repository").value = (
+ git_ref.repository.identity)
+ browser.getControl(name="field.git_ref.path").value = git_ref.path
+ browser.getControl("Create OCI recipe").click()
+ self.assertIn("Branch does not match format", browser.contents)
+
def test_create_new_recipe_with_build_args(self):
oci_project = self.factory.makeOCIProject()
- [git_ref] = self.factory.makeGitRefs()
+ [git_ref] = self.factory.makeGitRefs(
+ paths=['/refs/heads/v2.0-20.04'])
browser = self.getViewBrowser(
oci_project, view_name="+new-recipe", user=self.person)
browser.getControl(name="field.name").value = "recipe-name"
@@ -289,7 +305,8 @@ class TestOCIRecipeAddView(BaseTestOCIRecipeView):
def test_create_new_recipe_processors(self):
self.setUpDistroSeries()
oci_project = self.factory.makeOCIProject(pillar=self.distribution)
- [git_ref] = self.factory.makeGitRefs()
+ [git_ref] = self.factory.makeGitRefs(
+ paths=['/refs/heads/v2.0-20.04'])
browser = self.getViewBrowser(
oci_project, view_name="+new-recipe", user=self.person)
processors = browser.getControl(name="field.processors")
@@ -358,7 +375,8 @@ class TestOCIRecipeAddView(BaseTestOCIRecipeView):
distribution = self.factory.makeDistribution(
oci_project_admin=self.person)
oci_project = self.factory.makeOCIProject(pillar=distribution)
- [git_ref] = self.factory.makeGitRefs()
+ [git_ref] = self.factory.makeGitRefs(
+ paths=['/refs/heads/v2.0-20.04'])
browser = self.getViewBrowser(
oci_project, view_name="+new-recipe", user=self.person)
browser.getControl(name="field.name").value = "recipe-name"
@@ -381,11 +399,13 @@ class TestOCIRecipeAddView(BaseTestOCIRecipeView):
# do it once
oci_project = self.factory.makeOCIProject(pillar=distribution)
- [git_ref] = self.factory.makeGitRefs()
+ [git_ref] = self.factory.makeGitRefs(
+ paths=['/refs/heads/v2.0-20.04'])
# and then do it again
oci_project2 = self.factory.makeOCIProject(pillar=distribution)
- [git_ref2] = self.factory.makeGitRefs()
+ [git_ref2] = self.factory.makeGitRefs(
+ paths=['/refs/heads/v3.0-20.04'])
browser = self.getViewBrowser(
oci_project, view_name="+new-recipe", user=self.person)
browser.getControl(name="field.name").value = "recipe-name"
@@ -429,7 +449,8 @@ class TestOCIRecipeAddView(BaseTestOCIRecipeView):
distribution = self.factory.makeDistribution(
oci_project_admin=distro_owner)
oci_project = self.factory.makeOCIProject(pillar=distribution)
- [git_ref] = self.factory.makeGitRefs()
+ [git_ref] = self.factory.makeGitRefs(
+ paths=['/refs/heads/v2.0-20.04'])
browser = self.getViewBrowser(
oci_project, view_name="+new-recipe", user=self.person)
browser.getControl(name="field.name").value = "recipe-name"
@@ -448,7 +469,8 @@ class TestOCIRecipeAddView(BaseTestOCIRecipeView):
def test_create_recipe_doesnt_override_gitref_errors(self):
oci_project = self.factory.makeOCIProject()
- [git_ref] = self.factory.makeGitRefs()
+ [git_ref] = self.factory.makeGitRefs(
+ paths=['/refs/heads/v2.0-20.04'])
browser = self.getViewBrowser(
oci_project, view_name="+new-recipe", user=self.person)
browser.getControl(name="field.name").value = "recipe-name"
@@ -561,7 +583,8 @@ class TestOCIRecipeEditView(OCIConfigHelperMixin, BaseTestOCIRecipeView):
oci_project=oci_project, git_ref=old_git_ref)
self.factory.makeTeam(
name="new-team", displayname="New Team", members=[self.person])
- [new_git_ref] = self.factory.makeGitRefs()
+ [new_git_ref] = self.factory.makeGitRefs(
+ paths=['/refs/heads/v2.0-20.04'])
self.factory.makeOCIPushRule(recipe=recipe)
browser = self.getViewBrowser(recipe, user=self.person)
@@ -596,6 +619,25 @@ class TestOCIRecipeEditView(OCIConfigHelperMixin, BaseTestOCIRecipeView):
"Build schedule:\nBuilt daily\nEdit OCI recipe\n",
MatchesTagText(content, "build-schedule"))
+ def test_edit_recipe_invalid_branch(self):
+ oci_project = self.factory.makeOCIProject()
+ repository = self.factory.makeGitRepository()
+ [old_git_ref] = self.factory.makeGitRefs(repository=repository)
+ recipe = self.factory.makeOCIRecipe(
+ registrant=self.person, owner=self.person,
+ oci_project=oci_project, git_ref=old_git_ref)
+ self.factory.makeTeam(
+ name="new-team", displayname="New Team", members=[self.person])
+ [new_git_ref] = self.factory.makeGitRefs(
+ repository=repository, paths=['/refs/heads/invalid'])
+ self.factory.makeOCIPushRule(recipe=recipe)
+
+ browser = self.getViewBrowser(recipe, user=self.person)
+ browser.getLink("Edit OCI recipe").click()
+ browser.getControl(name="field.git_ref.path").value = new_git_ref.path
+ browser.getControl("Update OCI recipe").click()
+ self.assertIn("Branch does not match format", browser.contents)
+
def test_edit_recipe_sets_date_last_modified(self):
# Editing an OCI recipe sets the date_last_modified property.
date_created = datetime(2000, 1, 1, tzinfo=pytz.UTC)
diff --git a/lib/lp/oci/model/ocirecipe.py b/lib/lp/oci/model/ocirecipe.py
index fd14762..41493ee 100644
--- a/lib/lp/oci/model/ocirecipe.py
+++ b/lib/lp/oci/model/ocirecipe.py
@@ -12,7 +12,6 @@ __all__ = [
'OCIRecipeSet',
]
-import re
from lazr.lifecycle.event import ObjectCreatedEvent
import pytz
@@ -47,6 +46,7 @@ from zope.security.proxy import (
from lp.app.interfaces.launchpad import ILaunchpadCelebrities
from lp.app.interfaces.security import IAuthorization
+from lp.app.validators.validation import validate_oci_branch_name
from lp.buildmaster.enums import BuildStatus
from lp.buildmaster.interfaces.buildqueue import IBuildQueueSet
from lp.buildmaster.model.buildfarmjob import BuildFarmJob
@@ -201,25 +201,7 @@ class OCIRecipe(Storm, WebhookTargetMixin):
@property
def is_valid_branch_format(self):
- name = self.git_ref.name
- split = name.split('-')
- # if we've not got at least two components
- if len(split) < 2:
- return False
- app_version = split[0:-1]
- ubuntu_version = split[-1]
- # 20.04 format
- ubuntu_match = re.match("\d{2}\.\d{2}", ubuntu_version)
- if not ubuntu_match:
- return False
- # disallow risks in app version number
- for risk in ["stable", "candidate", "beta", "edge"]:
- if risk in app_version:
- return False
- # no '/' as they're a delimiter
- if '/' in app_version:
- return False
- return True
+ return validate_oci_branch_name(self.git_ref.name)
@property
def build_args(self):
diff --git a/lib/lp/testing/factory.py b/lib/lp/testing/factory.py
index 0bd8f83..df269c0 100644
--- a/lib/lp/testing/factory.py
+++ b/lib/lp/testing/factory.py
@@ -4937,7 +4937,9 @@ class BareLaunchpadObjectFactory(ObjectFactory):
if oci_project is None:
oci_project = self.makeOCIProject()
if git_ref is None:
- [git_ref] = self.makeGitRefs()
+ component = self.getUniqueUnicode()
+ paths = [u'refs/heads/{}-20.04'.format(component)]
+ [git_ref] = self.makeGitRefs(paths=paths)
if build_file is None:
build_file = self.getUniqueUnicode(u"build_file_for")
if build_path is None:
