launchpad-reviewers team mailing list archive

[Alvaro Crespo Serrano <mp+487377@xxxxxxxxxxxxxxxxxx>]

comments addressed, I did some research to answer some of the questions

Diff comments:

> diff --git a/Makefile b/Makefile
> index faca702..dea96d8 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -22,7 +22,7 @@ ifndef GITHASH
>  endif
>  GITTAGS ?= $(shell git tag)
>  
> -DEPENDENCY_REPO ?= lp:~launchpad/lp-signing/+git/dependencies
> +DEPENDENCY_REPO ?= https://git.launchpad.net/~alvarocs/lp-signing/+git/dependencies

updated to point to the correct repo https://git.launchpad.net/~launchpad/lp-signing/+git/dependencies

>  DEPENDENCY_DIR ?= $(TMPDIR)/dependencies
>  
>  # Create archives in labelled directories (e.g.
> diff --git a/lp_signing/auth.py b/lp_signing/auth.py
> index c35080d..3b6d8fe 100644
> --- a/lp_signing/auth.py
> +++ b/lp_signing/auth.py
> @@ -98,7 +99,12 @@ class BoxedRequest(Request):
>                  "Cannot decode {} header".format(header_name)
>              )
>  
> -    def _get_data_for_json(self, cache):

explanation of why we needed this change in the description of the mp

> +    def get_data(
> +        self,
> +        cache=True,
> +        as_text=False,
> +        parse_form_data=False,
> +    ):
>          """Read, authenticate, and decrypt incoming data.
>  
>          This may call `Nonce.check`, so the caller must only call this
> diff --git a/lp_signing/model/tests/test_client.py b/lp_signing/model/tests/test_client.py
> index 1e72d65..36b4105 100644
> --- a/lp_signing/model/tests/test_client.py
> +++ b/lp_signing/model/tests/test_client.py
> @@ -27,7 +27,7 @@ class TestClient(TestCase):
>          client.registerPublicKey(private_keys[0].public_key)
>          self.assertEqual([private_keys[0].public_key], client.public_keys)
>          client.registerPublicKey(private_keys[1].public_key)
> -        self.assertItemsEqual(
> +        self.assertCountEqual(

I've just rechecked this and when using assertItemsEqual you get a deprecation warning:

"lp-signing/lp_signing/model/tests/test_key.py:716: DeprecationWarning: Please use assertCountEqual instead.
  self.assertItemsEqual(clients, key.authorizations)
"

I agree the renaming is misleading, but 'assertCountEqual' checks exactly the same as 'assertItemsEqual' https://docs.python.org/3/library/unittest.html#unittest.TestCase.assertCountEqual

>              [private_key.public_key for private_key in private_keys],
>              client.public_keys,
>          )
> diff --git a/lp_signing/tests/test_webapi.py b/lp_signing/tests/test_webapi.py
> index d997724..bc22156 100644
> --- a/lp_signing/tests/test_webapi.py
> +++ b/lp_signing/tests/test_webapi.py
> @@ -3006,7 +3006,7 @@ class TestInjectView(TestCase):
>                  Path(tmp), KeyType.FIT, common_name
>              )
>  
> -        existent_date = datetime.utcnow()
> +        existent_date = datetime.now()

updated to utc: existent_date = datetime.now(timezone.utc)

>          resp = self.post_inject(
>              {
>                  "key-type": "FIT",
> diff --git a/lp_signing/webapp.py b/lp_signing/webapp.py
> index ed39e4c..a39f9e6 100644
> --- a/lp_signing/webapp.py
> +++ b/lp_signing/webapp.py
> @@ -100,8 +100,8 @@ def create_web_application():
>                          str(response.status_code),
>                      )
>                  )
> -                flask._request_ctx_stack.top.timer.stat = name
> -                flask._request_ctx_stack.top.timer.stop()
> +                flask.g.timer.stat = name
> +                flask.g.timer.stop()

looking at the logs when running tests:

"lp-signing/lp_signing/webapp.py:87: DeprecationWarning: '_request_ctx_stack' is deprecated and will be removed in Flask 2.4. Use 'g' to store data, or 'request_ctx' to access the current context.
  flask._request_ctx_stack.top.timer = statsd_client.timer("")"

The tests pass with both options, but '._request_ctx_stack' will be removed from Flask >2.4 so I have updated it to use the suggested 'g.' and avoid the deprecation warnings.

>          return response
>  
>      FlaskStorm().init_app(app)
> diff --git a/setup.py b/setup.py
> index f5b5f71..296bb76 100755
> --- a/setup.py
> +++ b/setup.py
> @@ -25,11 +25,12 @@ test_requires = [
>      "systemfixtures",
>      "testresources",
>      "testtools",
> +    "psycopg2-binary>=2.9,<3",

I was missing 'libpq-dev', and even running tox in the old bionic repo was failing for me to build psycopg2, that is why I added the binary and this workaround. I am reverting back to the 'talisker[flask,gunicorn,pg]' config as this works and does not need any change. I am also removing the psycopg binary from the dependencies folder as it is not needed.

>  ]
>  
>  setup(
>      name="lp-signing",
> -    version="0.1",
> +    version="0.2",
>      packages=find_packages(),
>      include_package_data=True,
>      zip_safe=False,
> @@ -46,7 +48,6 @@ setup(
>          "Programming Language :: Python",
>      ],
>      install_requires=requires,
> -    tests_require=test_requires,

adding this back, old setup.py works

>      extras_require={
>          "docs": ["sphinx"],
>          "test": test_requires,


-- 
https://code.launchpad.net/~alvarocs/lp-signing/+git/lp-signing-1/+merge/487377
Your team Launchpad code reviewers is requested to review the proposed merge of ~alvarocs/lp-signing/+git/lp-signing-1:upgrade-noble into lp-signing:master.