Merge ~pappacena/launchpad:public-ecr-aws-ui into launchpad:master

[Thiago F. Pappacena]

UI to allow adding region to OCI registry credentials

[Thiago F. Pappacena (pappacena)]

Screenshots can be found here:

https://private-fileshare.canonical.com/~pappacena/screenshots/oci/1.png
https://private-fileshare.canonical.com/~pappacena/screenshots/oci/2.png
https://private-fileshare.canonical.com/~pappacena/screenshots/oci/3.png

[Tom Wardill (twom)]

I think the field marked 'Region' should say 'Optional' or something. It's not clear (as a new user) that pushing to ROCKs or Dockerhub shouldn't need a 'region' field.

Also, I think it should appear in the overall push rules (picture 1), as you may have an image being pushed to multiple regions, with otherwise identical fields.

review: Needs Fixing

[Tom Wardill (twom)]

review: Approve

diff --git a/lib/lp/oci/browser/ocirecipe.py b/lib/lp/oci/browser/ocirecipe.py
index 460b108..b3a43d7 100644
--- a/lib/lp/oci/browser/ocirecipe.py
+++ b/lib/lp/oci/browser/ocirecipe.py
@@ -344,7 +344,9 @@ class OCIRecipeEditPushRulesView(LaunchpadFormView):
         super(OCIRecipeEditPushRulesView, self).setUpFields()
         image_name_fields = []
         url_fields = []
+        region_fields = []
         private_url_fields = []
+        private_region_fields = []
         username_fields = []
         private_username_fields = []
         password_fields = []
@@ -361,6 +363,13 @@ class OCIRecipeEditPushRulesView(LaunchpadFormView):
                         __name__=self._getFieldName('url', elem.id),
                         default=elem.registry_credentials.url,
                         required=True, readonly=True))
+                region = elem.registry_credentials.getCredentialsValue(
+                    'region')
+                region_fields.append(
+                    TextLine(
+                        __name__=self._getFieldName('region', elem.id),
+                        default=region,
+                        required=False, readonly=True))
                 username_fields.append(
                     TextLine(
                         __name__=self._getFieldName('username', elem.id),
@@ -380,6 +389,10 @@ class OCIRecipeEditPushRulesView(LaunchpadFormView):
                     TextLine(
                         __name__=self._getFieldName('url', elem.id),
                         default='', required=True, readonly=True))
+                private_region_fields.append(
+                    TextLine(
+                        __name__=self._getFieldName('region', elem.id),
+                        default='', required=False, readonly=True))
                 private_username_fields.append(
                     TextLine(
                         __name__=self._getFieldName('username', elem.id),
@@ -405,6 +418,10 @@ class OCIRecipeEditPushRulesView(LaunchpadFormView):
             TextLine(
                 __name__=u'add_url',
                 required=False, readonly=False))
+        region_fields.append(
+            TextLine(
+                __name__=u'add_region',
+                required=False, readonly=False))
         username_fields.append(
             TextLine(
                 __name__=u'add_username',
@@ -424,6 +441,10 @@ class OCIRecipeEditPushRulesView(LaunchpadFormView):
             FormFields(
                 *private_url_fields,
                 custom_widget=InvisibleCredentialsWidget) +
+            FormFields(*region_fields) +
+            FormFields(
+                *private_region_fields,
+                custom_widget=InvisibleCredentialsWidget) +
             FormFields(*username_fields) +
             FormFields(
                 *private_username_fields,
@@ -453,6 +474,8 @@ class OCIRecipeEditPushRulesView(LaunchpadFormView):
         widgets_by_name = {widget.name: widget for widget in self.widgets}
         url_field_name = (
                 "field." + self._getFieldName("url", rule.id))
+        region_field_name = (
+                "field." + self._getFieldName("region", rule.id))
         image_field_name = (
                 "field." + self._getFieldName("image_name", rule.id))
         username_field_name = (
@@ -461,6 +484,7 @@ class OCIRecipeEditPushRulesView(LaunchpadFormView):
                 "field." + self._getFieldName("delete", rule.id))
         return {
             "url": widgets_by_name[url_field_name],
+            "region": widgets_by_name[region_field_name],
             "image_name": widgets_by_name[image_field_name],
             "username": widgets_by_name[username_field_name],
             "delete": widgets_by_name[delete_field_name],
@@ -473,6 +497,7 @@ class OCIRecipeEditPushRulesView(LaunchpadFormView):
             "existing_credentials":
                 widgets_by_name["field.existing_credentials"],
             "url": widgets_by_name["field.add_url"],
+            "region": widgets_by_name["field.add_region"],
             "username": widgets_by_name["field.add_username"],
             "password": widgets_by_name["field.add_password"],
             "confirm_password": widgets_by_name["field.add_confirm_password"],
@@ -483,18 +508,20 @@ class OCIRecipeEditPushRulesView(LaunchpadFormView):
         parsed_data = {}
         add_image_name = data.get("add_image_name")
         add_url = data.get("add_url")
+        add_region = data.get("add_region")
         add_username = data.get("add_username")
         add_password = data.get("add_password")
         add_confirm_password = data.get("add_confirm_password")
         add_existing_credentials = data.get("existing_credentials")
 
         # parse data from the Add new rule section of the form
-        if (add_url or add_username or add_password or
+        if (add_url or add_region or add_username or add_password or
                 add_confirm_password or add_image_name or
                 add_existing_credentials):
             parsed_data.setdefault(None, {
                 "image_name": add_image_name,
                 "url": add_url,
+                "region": add_region,
                 "username": add_username,
                 "password": add_password,
                 "confirm_password": add_confirm_password,
@@ -523,6 +550,7 @@ class OCIRecipeEditPushRulesView(LaunchpadFormView):
         add_data = parsed_data[None]
         image_name = add_data.get("image_name")
         url = add_data.get("url")
+        region = add_data.get("region")
         password = add_data.get("password")
         confirm_password = add_data.get("confirm_password")
         username = add_data.get("username")
@@ -550,9 +578,12 @@ class OCIRecipeEditPushRulesView(LaunchpadFormView):
 
             credentials_set = getUtility(IOCIRegistryCredentialsSet)
             try:
+                credential_data = {'username': username, 'password': password}
+                if region is not None:
+                    credential_data['region'] = region
                 credentials = credentials_set.getOrCreate(
                     registrant=self.user, owner=self.context.owner, url=url,
-                    credentials={'username': username, 'password': password})
+                    credentials=credential_data)
             except OCIRegistryCredentialsAlreadyExist:
                 self.setFieldError(
                     "add_url",
diff --git a/lib/lp/oci/browser/tests/test_ocirecipe.py b/lib/lp/oci/browser/tests/test_ocirecipe.py
index 038e602..0ab9a59 100644
--- a/lib/lp/oci/browser/tests/test_ocirecipe.py
+++ b/lib/lp/oci/browser/tests/test_ocirecipe.py
@@ -1111,6 +1111,10 @@ class TestOCIRecipeEditPushRulesView(OCIConfigHelperMixin,
                                      text=soupmatchers._not_passed)),
                 soupmatchers.Within(
                     row,
+                    soupmatchers.Tag("Region", "td",
+                                     text=soupmatchers._not_passed)),
+                soupmatchers.Within(
+                    row,
                     soupmatchers.Tag("Username", "td",
                                      text=soupmatchers._not_passed)),
                 soupmatchers.Within(
@@ -1330,6 +1334,7 @@ class TestOCIRecipeEditPushRulesView(OCIConfigHelperMixin,
         browser.getControl(name="field.add_credentials").value = "new"
         browser.getControl(name="field.add_image_name").value = "imagename3"
         browser.getControl(name="field.add_url").value = url
+        browser.getControl(name="field.add_region").value = "somewhere-02"
         browser.getControl(name="field.add_username").value = "username"
         browser.getControl(name="field.add_password").value = "password"
         browser.getControl(
@@ -1347,8 +1352,9 @@ class TestOCIRecipeEditPushRulesView(OCIConfigHelperMixin,
                 url=url,
                 username="username")))
         with person_logged_in(self.person):
-            self.assertEqual(
-                {"username": "username", "password": "password"},
+            self.assertEqual({
+                "username": "username", "password": "password",
+                "region": "somewhere-02"},
                 rule.registry_credentials.getCredentials())
 
     def test_add_oci_push_rules_existing_credentials_duplicate(self):
@@ -1494,6 +1500,7 @@ class TestOCIRecipeEditPushRulesView(OCIConfigHelperMixin,
         browser.getLink("Edit OCI registry credentials").click()
 
         browser.getControl(name="field.add_url").value = url
+        browser.getControl(name="field.add_region").value = "new_region1"
         browser.getControl(name="field.add_username").value = "new_username"
         browser.getControl(name="field.add_password").value = "password"
         browser.getControl(name="field.add_confirm_password"
@@ -1508,8 +1515,10 @@ class TestOCIRecipeEditPushRulesView(OCIConfigHelperMixin,
             self.assertEqual(url, creds[1].url)
             self.assertThat(
                 (creds[1]).getCredentials(),
-                MatchesDict({"username": Equals("new_username"),
-                             "password": Equals("password")}))
+                MatchesDict({
+                    "username": Equals("new_username"),
+                    "password": Equals("password"),
+                    "region": Equals("new_region1")}))
 
 
 class TestOCIProjectRecipesView(BaseTestOCIRecipeView):
diff --git a/lib/lp/oci/javascript/ocirecipe.edit.js b/lib/lp/oci/javascript/ocirecipe.edit.js
index 8b3d82d..c31f532 100644
--- a/lib/lp/oci/javascript/ocirecipe.edit.js
+++ b/lib/lp/oci/javascript/ocirecipe.edit.js
@@ -23,6 +23,7 @@ YUI.add('lp.oci.ocirecipe.edit', function(Y) {
         });
         module.set_enabled('field.existing_credentials', value === 'existing');
         module.set_enabled('field.add_url', value === 'new');
+        module.set_enabled('field.add_region', value === 'new');
         module.set_enabled('field.add_username', value === 'new');
         module.set_enabled('field.add_password', value === 'new');
         module.set_enabled('field.add_confirm_password', value === 'new');
diff --git a/lib/lp/oci/templates/ocirecipe-edit-push-rules.pt b/lib/lp/oci/templates/ocirecipe-edit-push-rules.pt
index eee7db7..2cb3fec 100644
--- a/lib/lp/oci/templates/ocirecipe-edit-push-rules.pt
+++ b/lib/lp/oci/templates/ocirecipe-edit-push-rules.pt
@@ -46,6 +46,12 @@
           <tr>
             <th class="push-rule-image-name">Image name</th>
             <th class="push-rule-url">Registry URL</th>
+            <th class="push-rule-region">
+              Region (optional)
+              <img height="14" width="14" alt=""
+                   title="For AWS ECR, insert the AWS region of the repository. Leave blank otherwise."
+                   src="/@@/question" />
+            </th>
             <th class="push-rule-username">Username</th>
             <th class="push-rule-password">Password</th>
             <th class="push-rule-confirm-password">Confirm password</th>
@@ -66,6 +72,10 @@
                     tal:define="widget nocall:rule_widgets/url">
                   <metal:widget use-macro="context/@@launchpad_form/widget_div" />
                 </td>
+                <td class="push-rule-region"
+                    tal:define="widget nocall:rule_widgets/region">
+                  <metal:widget use-macro="context/@@launchpad_form/widget_div" />
+                </td>
                 <td class="push-rule-username"
                     tal:define="widget nocall:rule_widgets/username">
                   <metal:widget use-macro="context/@@launchpad_form/widget_div" />
@@ -115,6 +125,10 @@
                   tal:define="widget nocall:new_rule_widgets/url">
                 <metal:widget use-macro="context/@@launchpad_form/widget_div" />
               </td>
+              <td class="push-rule-region"
+                  tal:define="widget nocall:new_rule_widgets/region">
+                <metal:widget use-macro="context/@@launchpad_form/widget_div" />
+              </td>
               <td class="push-rule-username"
                   tal:define="widget nocall:new_rule_widgets/username">
                 <metal:widget use-macro="context/@@launchpad_form/widget_div" />
diff --git a/lib/lp/registry/browser/person.py b/lib/lp/registry/browser/person.py
index aef4eb0..55596e9 100644
--- a/lib/lp/registry/browser/person.py
+++ b/lib/lp/registry/browser/person.py
@@ -3738,17 +3738,25 @@ class PersonEditOCIRegistryCredentialsView(LaunchpadFormView):
             default=credentials.url,
             required=True, readonly=False)
 
+        region = TextLine(
+            __name__=self._getFieldName('region', id),
+            default=credentials.getCredentialsValue("region"),
+            required=False, readonly=False)
+
         delete = Bool(
             __name__=self._getFieldName('delete', id),
             default=False,
             required=True, readonly=False)
 
-        return owner, username, password, confirm_password, url, delete
+        return owner, username, password, confirm_password, url, region, delete
 
     def getAddFieldsRow(self):
         add_url = TextLine(
             __name__=u'add_url',
             required=False, readonly=False)
+        add_region = TextLine(
+            __name__=u'add_region',
+            required=False, readonly=False)
         add_owner = Choice(
             __name__=u'add_owner',
             vocabulary=(
@@ -3766,7 +3774,7 @@ class PersonEditOCIRegistryCredentialsView(LaunchpadFormView):
             required=False, readonly=False)
 
         return (
-            add_url, add_owner, add_username,
+            add_url, add_region, add_owner, add_username,
             add_password, add_confirm_password)
 
     def _parseFieldName(self, field_name):
@@ -3825,6 +3833,8 @@ class PersonEditOCIRegistryCredentialsView(LaunchpadFormView):
                 "field." + self._getFieldName("confirm_password",
                                               credentials.id))
         url_field_name = "field." + self._getFieldName("url", credentials.id)
+        region_field_name = "field." + self._getFieldName(
+            "region", credentials.id)
         delete_field_name = (
                 "field." + self._getFieldName("delete", credentials.id))
         return {
@@ -3833,6 +3843,7 @@ class PersonEditOCIRegistryCredentialsView(LaunchpadFormView):
             "password": widgets_by_name[password_field_name],
             "confirm_password": widgets_by_name[confirm_password_field_name],
             "url": widgets_by_name[url_field_name],
+            "region": widgets_by_name[region_field_name],
             "delete": widgets_by_name[delete_field_name]
         }
 
@@ -3840,6 +3851,7 @@ class PersonEditOCIRegistryCredentialsView(LaunchpadFormView):
         """Rearrange form data to make it easier to process."""
         parsed_data = {}
         add_url = data["add_url"]
+        add_region = data["add_region"]
         add_owner = data["add_owner"]
         add_username = data["add_username"]
         add_password = data["add_password"]
@@ -3850,6 +3862,7 @@ class PersonEditOCIRegistryCredentialsView(LaunchpadFormView):
                 "password": add_password,
                 "confirm_password": add_confirm_password,
                 "url": add_url,
+                "region": add_region,
                 "owner": add_owner,
                 "action": "add",
             })
@@ -3865,6 +3878,7 @@ class PersonEditOCIRegistryCredentialsView(LaunchpadFormView):
             confirm_password_field_name = self._getFieldName(
                 "confirm_password", credentials_id)
             url_field_name = self._getFieldName("url", credentials_id)
+            region_field_name = self._getFieldName("region", credentials_id)
             delete_field_name = self._getFieldName("delete", credentials_id)
             if data.get(delete_field_name):
                 action = "delete"
@@ -3875,6 +3889,7 @@ class PersonEditOCIRegistryCredentialsView(LaunchpadFormView):
                 "password": data.get(password_field_name),
                 "confirm_password": data.get(confirm_password_field_name),
                 "url": data.get(url_field_name),
+                "region": data.get(region_field_name),
                 "owner": data.get(owner_field_name),
                 "action": action,
             })
@@ -3882,6 +3897,7 @@ class PersonEditOCIRegistryCredentialsView(LaunchpadFormView):
         return parsed_data
 
     def changeCredentials(self, parsed_credentials, credentials):
+        region = parsed_credentials["region"]
         username = parsed_credentials["username"]
         password = parsed_credentials["password"]
         confirm_password = parsed_credentials["confirm_password"]
@@ -3902,6 +3918,11 @@ class PersonEditOCIRegistryCredentialsView(LaunchpadFormView):
             credentials.url = parsed_credentials["url"]
         elif parsed_credentials["url"] != credentials.url:
             credentials.url = parsed_credentials["url"]
+        if credentials.getCredentialsValue('region') != region:
+            credentials_data = removeSecurityProxy(
+                credentials.getCredentials())
+            credentials_data["region"] = region
+            credentials.setCredentials(credentials_data)
         if owner != credentials.owner:
             credentials.owner = owner
 
@@ -3919,6 +3940,7 @@ class PersonEditOCIRegistryCredentialsView(LaunchpadFormView):
 
     def addCredentials(self, parsed_add_credentials):
         url = parsed_add_credentials["url"]
+        region = parsed_add_credentials["region"]
         owner = parsed_add_credentials["owner"]
         password = parsed_add_credentials["password"]
         confirm_password = parsed_add_credentials["confirm_password"]
@@ -3936,6 +3958,8 @@ class PersonEditOCIRegistryCredentialsView(LaunchpadFormView):
                 credentials = {
                     'username': username,
                     'password': password}
+                if region:
+                    credentials["region"] = region
                 try:
                     getUtility(IOCIRegistryCredentialsSet).new(
                         registrant=self.user, owner=owner, url=url,
diff --git a/lib/lp/registry/browser/tests/test_person.py b/lib/lp/registry/browser/tests/test_person.py
index 07ef8d4..350afcb 100644
--- a/lib/lp/registry/browser/tests/test_person.py
+++ b/lib/lp/registry/browser/tests/test_person.py
@@ -1455,13 +1455,16 @@ class TestPersonOCIRegistryCredentialsView(
                         "password": Equals("bar"),
                         })))
 
-        # change only the registry url
+        # change only the registry url and region
         browser = self.getViewBrowser(
             self.owner, view_name='+oci-registry-credentials', user=self.user)
         browser.getLink("Edit OCI registry credentials").click()
         url_control = browser.getControl(
             name="field.url.%d" % registry_credentials_id)
         url_control.value = newurl
+        url_control = browser.getControl(
+            name="field.region.%d" % registry_credentials_id)
+        url_control.value = 'us-west-2'
         browser.getControl("Save").click()
         with person_logged_in(self.user):
             self.assertThat(
@@ -1470,6 +1473,7 @@ class TestPersonOCIRegistryCredentialsView(
                     MatchesDict({
                         "username": Equals("different_username"),
                         "password": Equals("bar"),
+                        "region": Equals("us-west-2"),
                         })))
 
         # change only the password
@@ -1484,7 +1488,7 @@ class TestPersonOCIRegistryCredentialsView(
         self.assertIn(
             "Passwords do not match.", six.ensure_text(browser.contents))
 
-        # change all fields with one edit action
+        # change all fields (except region) with one edit action
         username_control = browser.getControl(
             name="field.username.%d" % registry_credentials_id)
         username_control.value = 'third_different_username'
@@ -1506,6 +1510,7 @@ class TestPersonOCIRegistryCredentialsView(
                     MatchesDict({
                         "username": Equals("third_different_username"),
                         "password": Equals("third_newpassword"),
+                        "region": Equals("us-west-2"),
                         })))
 
     def test_add_oci_registry_credentials(self):
@@ -1530,6 +1535,7 @@ class TestPersonOCIRegistryCredentialsView(
             [owner_name], browser.getControl(name="field.add_owner").value)
 
         browser.getControl(name="field.add_url").value = url
+        browser.getControl(name="field.add_region").value = "sa-east-1"
         browser.getControl(name="field.add_owner").value = [new_owner_name]
         browser.getControl(name="field.add_username").value = "new_username"
         browser.getControl(name="field.add_password").value = "password"
@@ -1555,6 +1561,7 @@ class TestPersonOCIRegistryCredentialsView(
                         MatchesDict({
                             "username": Equals("new_username"),
                             "password": Equals("password"),
+                            "region": Equals("sa-east-1"),
                             }))))
 
     def test_delete_oci_registry_credentials(self):
diff --git a/lib/lp/registry/templates/person-edit-ociregistrycredentials.pt b/lib/lp/registry/templates/person-edit-ociregistrycredentials.pt
index 1b0fd0f..27aa9d3 100644
--- a/lib/lp/registry/templates/person-edit-ociregistrycredentials.pt
+++ b/lib/lp/registry/templates/person-edit-ociregistrycredentials.pt
@@ -43,6 +43,12 @@
     <thead>
       <tr>
         <th class="credentials-url">Registry URL</th>
+        <th class="credentials-region">
+          Region (optional)
+          <img height="14" width="14" alt=""
+               title="For AWS ECR, insert the AWS region of the repository. Leave blank otherwise."
+               src="/@@/question" />
+        </th>
         <th class="credentials-owner">Owner</th>
         <th class="credentials-username">Username</th>
         <th class="credentials-password">Password</th>
@@ -60,6 +66,10 @@
                 tal:define="widget nocall:credentials_widgets/url">
               <metal:widget use-macro="context/@@launchpad_form/widget_div" />
             </td>
+            <td class="credentials-region"
+                tal:define="widget nocall:credentials_widgets/region">
+              <metal:widget use-macro="context/@@launchpad_form/widget_div" />
+            </td>
             <td class="credentials-owner"
                 tal:define="widget nocall:credentials_widgets/owner">
               <metal:widget use-macro="context/@@launchpad_form/widget_div" />
@@ -91,6 +101,10 @@
               tal:define="widget nocall:view/widgets/add_url">
             <metal:widget use-macro="context/@@launchpad_form/widget_div" />
           </td>
+          <td class="credentials-region"
+              tal:define="widget nocall:view/widgets/add_region">
+            <metal:widget use-macro="context/@@launchpad_form/widget_div" />
+          </td>
           <td class="credentials-owner"
               tal:define="widget nocall:view/widgets/add_owner">
             <metal:widget use-macro="context/@@launchpad_form/widget_div" />