Launchpad itself

Merge ~twom/launchpad:oci-fix-tagging-tag-names into launchpad:master

  1. Git
  2. lp:~twom/launchpad
  3. oci-fix-tagging-tag-names
  4. Merge into master
Proposed by Tom Wardill
Status: Merged
Approved by: Tom Wardill
Approved revision: e2ab5696b9f6baf70c44896f3b20d8630e27b820
Merge reported by: Otto Co-Pilot
Merged at revision: not available
Proposed branch: ~twom/launchpad:oci-fix-tagging-tag-names
Merge into: launchpad:master
Diff against target: 232 lines (+76/-16)
6 files modified
lib/lp/app/validators/tests/test_validation.py (+41/-0)
lib/lp/app/validators/validation.py (+8/-2)
lib/lp/oci/browser/tests/test_ocirecipe.py (+12/-12)
lib/lp/oci/model/ocirecipe.py (+1/-1)
lib/lp/oci/model/ociregistryclient.py (+7/-1)
lib/lp/oci/tests/test_ociregistryclient.py (+7/-0)
Reviewer Review Type Date Requested Status
Colin Watson (community) Approve
Cristian Gonzalez (community) Approve
Review via email: mp+400377@code.launchpad.net

Commit message

Account for tag names in the correct branch format

Description of the change

If you manage to select a tag from the branch picker, then we shouldn't attempt to upload the manifest to an invalid location.

LP:1921865

To post a comment you must log in.
Revision history for this message
Cristian Gonzalez (cristiangsp) wrote :

Looks good!

review: Approve
Revision history for this message
Colin Watson (cjwatson) wrote :

This seems OK, though perhaps consider doing the validation on `GitRef.path` rather than `GitRef.name`: that way you could strip off both "refs/heads/" and "refs/tags/" in one place, and it would be IMO a bit less confusing. (It would also avoid being tripped up by pathological edge cases such as "refs/heads/refs/tags/v1.0-20.04".)

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1diff --git a/lib/lp/app/validators/tests/test_validation.py b/lib/lp/app/validators/tests/test_validation.py
2new file mode 100644
3index 0000000..7c9c8b5
4--- /dev/null
5+++ b/lib/lp/app/validators/tests/test_validation.py
6@@ -0,0 +1,41 @@
7+# Copyright 2021 Canonical Ltd. This software is licensed under the
8+# GNU Affero General Public License version 3 (see the file LICENSE).
9+
10+"""Unit tests for field validators"""
11+
12+from __future__ import absolute_import, print_function, unicode_literals
13+
14+__metaclass__ = type
15+
16+from lp.app.validators.validation import validate_oci_branch_name
17+from lp.testing import TestCase
18+from lp.testing.layers import BaseLayer
19+
20+
21+class TestOCIBranchValidator(TestCase):
22+
23+ layer = BaseLayer
24+
25+ def test_validate_oci_branch_name_with_leading_slash(self):
26+ self.assertFalse(validate_oci_branch_name('/refs/heads/v2.1.0-20.04'))
27+
28+ def test_validate_oci_branch_name_full(self):
29+ self.assertTrue(validate_oci_branch_name('refs/heads/v2.1.0-20.04'))
30+
31+ def test_validate_oci_branch_name_just_branch_name(self):
32+ self.assertTrue(validate_oci_branch_name('v2.1.0-20.04'))
33+
34+ def test_validate_oci_branch_name_failure(self):
35+ self.assertFalse(validate_oci_branch_name('notvalidbranch'))
36+
37+ def test_validate_oci_branch_name_invalid_ubuntu_version(self):
38+ self.assertFalse(validate_oci_branch_name('v2.1.0-ubuntu20.04'))
39+
40+ def test_validate_oci_branch_name_invalid_delimiter(self):
41+ self.assertFalse(validate_oci_branch_name('v2/1.0-20.04'))
42+
43+ def test_validate_oci_branch_name_tag(self):
44+ self.assertTrue(validate_oci_branch_name('refs/tags/v2-1.0-20.04'))
45+
46+ def test_validate_oci_branch_name_heads_and_tags(self):
47+ self.assertFalse(validate_oci_branch_name("refs/heads/refs/tags/v1.0-20.04"))
48diff --git a/lib/lp/app/validators/validation.py b/lib/lp/app/validators/validation.py
49index e9ef8b1..7ff8ad4 100644
50--- a/lib/lp/app/validators/validation.py
51+++ b/lib/lp/app/validators/validation.py
52@@ -86,6 +86,11 @@ def validate_new_team_email(email):
53
54 def validate_oci_branch_name(branch_name):
55 """Check that a git ref name matches appversion-ubuntuversion."""
56+ # Remove components to just get the branch/tag name
57+ if branch_name.startswith('refs/tags/'):
58+ branch_name = branch_name[len('refs/tags/'):]
59+ elif branch_name.startswith('refs/heads/'):
60+ branch_name = branch_name[len('refs/heads/'):]
61 split = branch_name.split('-')
62 # if we've not got at least two components
63 if len(split) < 2:
64@@ -101,6 +106,7 @@ def validate_oci_branch_name(branch_name):
65 if risk in app_version:
66 return False
67 # no '/' as they're a delimiter
68- if '/' in app_version:
69- return False
70+ for segment in app_version:
71+ if '/' in segment:
72+ return False
73 return True
74diff --git a/lib/lp/oci/browser/tests/test_ocirecipe.py b/lib/lp/oci/browser/tests/test_ocirecipe.py
75index 490a0e4..b06af3f 100644
76--- a/lib/lp/oci/browser/tests/test_ocirecipe.py
77+++ b/lib/lp/oci/browser/tests/test_ocirecipe.py
78@@ -192,7 +192,7 @@ class TestOCIRecipeAddView(OCIConfigHelperMixin, BaseTestOCIRecipeView):
79 oci_project = self.factory.makeOCIProject()
80 oci_project_display = oci_project.display_name
81 [git_ref] = self.factory.makeGitRefs(
82- paths=['/refs/heads/v2.0-20.04'])
83+ paths=['refs/heads/v2.0-20.04'])
84 source_display = git_ref.display_name
85 browser = self.getViewBrowser(
86 oci_project, view_name="+new-recipe", user=self.person)
87@@ -232,7 +232,7 @@ class TestOCIRecipeAddView(OCIConfigHelperMixin, BaseTestOCIRecipeView):
88 def test_create_new_recipe_invalid_format(self):
89 oci_project = self.factory.makeOCIProject()
90 [git_ref] = self.factory.makeGitRefs(
91- paths=['/refs/heads/invalid'])
92+ paths=['refs/heads/invalid'])
93 browser = self.getViewBrowser(
94 oci_project, view_name="+new-recipe", user=self.person)
95 browser.getControl(name="field.name").value = "recipe-name"
96@@ -246,7 +246,7 @@ class TestOCIRecipeAddView(OCIConfigHelperMixin, BaseTestOCIRecipeView):
97 def test_create_new_recipe_with_build_args(self):
98 oci_project = self.factory.makeOCIProject()
99 [git_ref] = self.factory.makeGitRefs(
100- paths=['/refs/heads/v2.0-20.04'])
101+ paths=['refs/heads/v2.0-20.04'])
102 browser = self.getViewBrowser(
103 oci_project, view_name="+new-recipe", user=self.person)
104 browser.getControl(name="field.name").value = "recipe-name"
105@@ -270,7 +270,7 @@ class TestOCIRecipeAddView(OCIConfigHelperMixin, BaseTestOCIRecipeView):
106 with person_logged_in(oci_project.distribution.owner):
107 oci_project.distribution.oci_registry_credentials = credentials
108 [git_ref] = self.factory.makeGitRefs(
109- paths=['/refs/heads/v2.0-20.04'])
110+ paths=['refs/heads/v2.0-20.04'])
111 browser = self.getViewBrowser(
112 oci_project, view_name="+new-recipe", user=self.person)
113 browser.getControl(name="field.name").value = "recipe-name"
114@@ -330,7 +330,7 @@ class TestOCIRecipeAddView(OCIConfigHelperMixin, BaseTestOCIRecipeView):
115 self.setUpDistroSeries()
116 oci_project = self.factory.makeOCIProject(pillar=self.distribution)
117 [git_ref] = self.factory.makeGitRefs(
118- paths=['/refs/heads/v2.0-20.04'])
119+ paths=['refs/heads/v2.0-20.04'])
120 browser = self.getViewBrowser(
121 oci_project, view_name="+new-recipe", user=self.person)
122 processors = browser.getControl(name="field.processors")
123@@ -400,7 +400,7 @@ class TestOCIRecipeAddView(OCIConfigHelperMixin, BaseTestOCIRecipeView):
124 oci_project_admin=self.person)
125 oci_project = self.factory.makeOCIProject(pillar=distribution)
126 [git_ref] = self.factory.makeGitRefs(
127- paths=['/refs/heads/v2.0-20.04'])
128+ paths=['refs/heads/v2.0-20.04'])
129 browser = self.getViewBrowser(
130 oci_project, view_name="+new-recipe", user=self.person)
131 browser.getControl(name="field.name").value = "recipe-name"
132@@ -424,12 +424,12 @@ class TestOCIRecipeAddView(OCIConfigHelperMixin, BaseTestOCIRecipeView):
133 # do it once
134 oci_project = self.factory.makeOCIProject(pillar=distribution)
135 [git_ref] = self.factory.makeGitRefs(
136- paths=['/refs/heads/v2.0-20.04'])
137+ paths=['refs/heads/v2.0-20.04'])
138
139 # and then do it again
140 oci_project2 = self.factory.makeOCIProject(pillar=distribution)
141 [git_ref2] = self.factory.makeGitRefs(
142- paths=['/refs/heads/v3.0-20.04'])
143+ paths=['refs/heads/v3.0-20.04'])
144 browser = self.getViewBrowser(
145 oci_project, view_name="+new-recipe", user=self.person)
146 browser.getControl(name="field.name").value = "recipe-name"
147@@ -474,7 +474,7 @@ class TestOCIRecipeAddView(OCIConfigHelperMixin, BaseTestOCIRecipeView):
148 oci_project_admin=distro_owner)
149 oci_project = self.factory.makeOCIProject(pillar=distribution)
150 [git_ref] = self.factory.makeGitRefs(
151- paths=['/refs/heads/v2.0-20.04'])
152+ paths=['refs/heads/v2.0-20.04'])
153 browser = self.getViewBrowser(
154 oci_project, view_name="+new-recipe", user=self.person)
155 browser.getControl(name="field.name").value = "recipe-name"
156@@ -494,7 +494,7 @@ class TestOCIRecipeAddView(OCIConfigHelperMixin, BaseTestOCIRecipeView):
157 def test_create_recipe_doesnt_override_gitref_errors(self):
158 oci_project = self.factory.makeOCIProject()
159 [git_ref] = self.factory.makeGitRefs(
160- paths=['/refs/heads/v2.0-20.04'])
161+ paths=['refs/heads/v2.0-20.04'])
162 browser = self.getViewBrowser(
163 oci_project, view_name="+new-recipe", user=self.person)
164 browser.getControl(name="field.name").value = "recipe-name"
165@@ -608,7 +608,7 @@ class TestOCIRecipeEditView(OCIConfigHelperMixin, BaseTestOCIRecipeView):
166 self.factory.makeTeam(
167 name="new-team", displayname="New Team", members=[self.person])
168 [new_git_ref] = self.factory.makeGitRefs(
169- paths=['/refs/heads/v2.0-20.04'])
170+ paths=['refs/heads/v2.0-20.04'])
171 self.factory.makeOCIPushRule(recipe=recipe)
172
173 browser = self.getViewBrowser(recipe, user=self.person)
174@@ -653,7 +653,7 @@ class TestOCIRecipeEditView(OCIConfigHelperMixin, BaseTestOCIRecipeView):
175 self.factory.makeTeam(
176 name="new-team", displayname="New Team", members=[self.person])
177 [new_git_ref] = self.factory.makeGitRefs(
178- repository=repository, paths=['/refs/heads/invalid'])
179+ repository=repository, paths=['refs/heads/invalid'])
180 self.factory.makeOCIPushRule(recipe=recipe)
181
182 browser = self.getViewBrowser(recipe, user=self.person)
183diff --git a/lib/lp/oci/model/ocirecipe.py b/lib/lp/oci/model/ocirecipe.py
184index e4edcae..7d665d0 100644
185--- a/lib/lp/oci/model/ocirecipe.py
186+++ b/lib/lp/oci/model/ocirecipe.py
187@@ -208,7 +208,7 @@ class OCIRecipe(Storm, WebhookTargetMixin):
188
189 @property
190 def is_valid_branch_format(self):
191- return validate_oci_branch_name(self.git_ref.name)
192+ return validate_oci_branch_name(self.git_ref.path)
193
194 @property
195 def build_args(self):
196diff --git a/lib/lp/oci/model/ociregistryclient.py b/lib/lp/oci/model/ociregistryclient.py
197index 34a8c1e..4d51843 100644
198--- a/lib/lp/oci/model/ociregistryclient.py
199+++ b/lib/lp/oci/model/ociregistryclient.py
200@@ -257,7 +257,13 @@ class OCIRegistryClient:
201 """
202 tags = []
203 if recipe.is_valid_branch_format:
204- tags.append("{}_{}".format(recipe.git_ref.name, "edge"))
205+ ref_name = recipe.git_ref.path
206+ # lp:1921865, account for tags in the correct format
207+ if ref_name.startswith('refs/tags/'):
208+ ref_name = ref_name[len('refs/tags/'):]
209+ elif ref_name.startswith('refs/heads/'):
210+ ref_name = ref_name[len('refs/heads/'):]
211+ tags.append("{}_{}".format(ref_name, "edge"))
212 else:
213 tags.append("edge")
214 return tags
215diff --git a/lib/lp/oci/tests/test_ociregistryclient.py b/lib/lp/oci/tests/test_ociregistryclient.py
216index 10f3af8..e30a470 100644
217--- a/lib/lp/oci/tests/test_ociregistryclient.py
218+++ b/lib/lp/oci/tests/test_ociregistryclient.py
219@@ -418,6 +418,13 @@ class TestOCIRegistryClient(OCIConfigHelperMixin, SpyProxyCallsMixin,
220 self.assertThat(result, MatchesListwise(
221 [Equals("v1.0-20.04_edge")]))
222
223+ def test_calculateTags_valid_tag(self):
224+ [git_ref] = self.factory.makeGitRefs(paths=["refs/tags/v1.0-20.04"])
225+ self.build.recipe.git_ref = git_ref
226+ result = self.client._calculateTags(self.build.recipe)
227+ self.assertThat(result, MatchesListwise(
228+ [Equals("v1.0-20.04_edge")]))
229+
230 def test_build_registry_manifest(self):
231 self._makeFiles()
232 preloaded_data = self.client._preloadFiles(

Subscribers

People subscribed via source and target branches

to status/vote changes: