← Back to team overview

launchpad-reviewers team mailing list archive

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

 

Review: Needs Fixing

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.

Diff comments:

> diff --git a/lib/lp/oci/browser/ocirecipe.py b/lib/lp/oci/browser/ocirecipe.py
> index 460b108..2103e1e 100644
> --- a/lib/lp/oci/browser/ocirecipe.py
> +++ b/lib/lp/oci/browser/ocirecipe.py
> @@ -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=True, readonly=True))

Is `required=True` correct here?

>                  private_username_fields.append(
>                      TextLine(
>                          __name__=self._getFieldName('username', elem.id),


-- 
https://code.launchpad.net/~pappacena/launchpad/+git/launchpad/+merge/394599
Your team Launchpad code reviewers is subscribed to branch ~pappacena/launchpad:public-ecr-aws.


References