launchpad-reviewers team mailing list archive

[Guruprasad <mp+487377@xxxxxxxxxxxxxxxxxx>]

It is a good idea to mention in the commit message that these changes are backwards-incompatible and the resulting application will not run on Python 3.6 + bionic.

Let me do the checks for the 2-3 items that I have mentioned below and then get back to you soon on that. If you have the time to do the same research and figure out answers to those questions, let me know.

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

This needs to be fixed before we merge as this is referring to your own fork.

>  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):

I will check this replacement and the below `if-else + for-else` ladder closely locally and then let you know if I have any questions or concerns.

> +    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(

Since the `assertItemsEqual` and `assertCountEqual` methods test different things (the count and actual items vs just the count), let me check and confirm if there is an alternative to use here.

>              [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()

I think this is not the correct replacement. Something like `datetime.now(timezone.utc)` that you have used above should be the right one.

>          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()

Let me check and confirm what this does and whether this is the right way or not.

>          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 am not sure that removing the `pg` extra from the `talisker` dependency above and including the `psycopg2-binary` wheel here instead is correct. The talisker `pg` extra's constraints do allow installing `psycopg2` 2.9 if that is what we want here. We just need to put that sdist tarball in the source dependencies repository.

>  ]
>  
>  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,

I don't think we should remove this even if it is mentioned in the `extra_require` value below. If you find any documentation or evidence that supports this removal, do share.

>      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.