Fixes looking around #281

fstagni · 2024-08-21T12:56:53Z

No description provided.

chaen · 2024-08-21T20:36:06Z

diracx-routers/tests/test_job_manager.py

@@ -569,7 +569,7 @@ def test_set_job_status_offset_naive_datetime_return_bad_request(
    valid_job_id: int,
 ):
    # Act
-    date = datetime.utcnow().isoformat(sep=" ")  # noqa: DTZ003


I think this was on purpose, given that there was the proper noqa https://docs.astral.sh/ruff/rules/call-datetime-utcnow/
I would guess it has to do with DIRAC compatibility but I don't remember

utcnow will be deprecated. What I put is of course equivalent.

Even though it is subtle, it is not

In [19]: tz_now = datetime.now(tz=timezone.utc); now = datetime.utcnow() In [20]: tz_now Out[20]: datetime.datetime(2024, 8, 22, 21, 13, 40, 232387, tzinfo=datetime.timezone.utc) In [21]: now Out[21]: datetime.datetime(2024, 8, 22, 21, 13, 40, 232433) In [22]: tz_now - now --------------------------------------------------------------------------- TypeError Traceback (most recent call last) Cell In[22], line 1 ----> 1 tz_now - now TypeError: can't subtract offset-naive and offset-aware datetimes

I was tricked by that in DIRAC in the past:

https://github.com/DIRACGrid/DIRAC/blob/f18be6f9d27f5055512f2c372718734270a060ae/src/DIRAC/Core/Security/m2crypto/asn1_utils.py#L196-L201

But indeed here we should check why was this done and maybe your fix is good.

Further converting it is OK:

In [9]: formatted__tz_now = tz_now.isoformat(sep=' ').split("+")[0] In [10]: formatted__now = now.isoformat(sep=' ').split("+")[0] In [14]: formatted__tz_now Out[14]: '2024-08-23 08:56:13.234331' In [15]: formatted__now Out[15]: '2024-08-23 08:56:13.234372' In [16]: date_obj__tz_now = datetime.strptime(formatted__tz_now, '%Y-%m-%d %H:%M:%S.%f') In [17]: date_obj__now = datetime.strptime(formatted__now, '%Y-%m-%d %H:%M:%S.%f') In [18]: date_obj__tz_now - date_obj__now Out[18]: datetime.timedelta(days=-1, seconds=86399, microseconds=999959)

But:

In [20]: date_obj__tz_now__no_split = datetime.strptime(formatted__tz_now__no_split, '%Y-%m-%d %H:%M:%S.%f') --------------------------------------------------------------------------- ValueError Traceback (most recent call last) Cell In[20], line 1 ----> 1 date_obj__tz_now__no_split = datetime.strptime(formatted__tz_now__no_split, '%Y-%m-%d %H:%M:%S.%f') File ~/miniforge3/envs/dirac-development/lib/python3.11/_strptime.py:567, in _strptime_datetime(cls, data_string, format) 564 def _strptime_datetime(cls, data_string, format="%a %b %d %H:%M:%S %Y"): 565 """Return a class cls instance based on the input string and the 566 format string.""" --> 567 tt, fraction, gmtoff_fraction = _strptime(data_string, format) 568 tzname, gmtoff = tt[-2:] 569 args = tt[:6] + (fraction,) File ~/miniforge3/envs/dirac-development/lib/python3.11/_strptime.py:352, in _strptime(data_string, format) 349 raise ValueError("time data %r does not match format %r" % 350 (data_string, format)) 351 if len(data_string) != found.end(): --> 352 raise ValueError("unconverted data remains: %s" % 353 data_string[found.end():]) 355 iso_year = year = None 356 month = day = 1 ValueError: unconverted data remains: +00:00 In [21]: formatted__tz_now__no_split Out[21]: '2024-08-23 08:56:13.234331+00:00' In [22]: date_obj__tz_now__no_split = datetime.strptime(formatted__tz_now__no_split, '%Y-%m-%d %H:%M:%S.%f+00:00') In [23]: date_obj__tz_now__no_split Out[23]: datetime.datetime(2024, 8, 23, 8, 56, 13, 234331) In [24]: date_obj__tz_now__no_split - date_obj__now Out[24]: datetime.timedelta(days=-1, seconds=86399, microseconds=999959)

...I guess it is a feature, but to me it even feels like a python bug! Bof!

.github/workflows/main.yml

chaen · 2024-08-23T08:34:19Z

Why removing the __future__ import ?

fstagni · 2024-08-23T09:28:24Z

Why removing the __future__ import ?

Because since python 3.7 is only necessary for specific cases.

chrisburr · 2024-08-23T11:20:25Z

Because since python 3.7 is only necessary for specific cases.

It was added in 3.7 so that can't be the case 😉

The intention is to make the from __future__ import annotations the default (see PEP-563 but it keeps getting deffered.

chaen · 2024-08-23T11:22:26Z

It actually should be everywhere except specific places like

diracx/diracx-routers/src/diracx/routers/access_policies.py

Lines 31 to 35 in b86ed11

    
           # FastAPI bug: 
        
           # We normally would use `from __future__ import annotations` 
        
           # but a bug in FastAPI prevents us from doing so 
        
           # https://github.com/tiangolo/fastapi/pull/11355 
        
           # Until it is merged, we can work around it by using strings.

fstagni · 2024-08-23T11:51:24Z

Sorry, I meant 3.10, but indeed I realise now that the decision to make it mandatory was postponed and it is not even in 3.12. So, I revert this change.

.github/workflows/semantic.yml

.github/workflows/main.yml

chrisburr · 2024-08-23T11:55:50Z

diracx-core/tests/test_secrets.py

@@ -15,8 +15,7 @@ def compare_keys(key1, key2):
 def test_token_signing_key(tmp_path):
    private_key = rsa.generate_private_key(
        public_exponent=65537,
-        # DANGER: 512-bits is a bad idea for prod but makes the test notably faster!
-        key_size=512,
+        key_size=1024,


Can we avoid making the tests slower by using elliptic curves?

In this PR? Can we leave for another one?

diracx-routers/tests/jobs/test_sandboxes.py

chaen · 2024-08-23T13:44:29Z

.github/workflows/main.yml

-            python-gfal2
-            mypy
-            pip
+          environment-file: environment.yml


it's not this environment.yml but the one from container-images that should be used

This environment.yml defines the environment where all of us are working locally, right? So, it's the natural environment where all unit tests run.

In container-images there are many environment.yml files. What is here is, permit me that, the superset of all of them.

absolutely, but this is what we want to try to avoid. We want to have the minimal set of dependencies installed to keep control of what we actually need, and this is why we did not blindly put the environment.yml before

So, you want to repeat, for ONLY the unit tests run by the CI, the ~same environment (because it is not going to be very different) that we all have in our local machines, and that we all use for running the unit tests locally? Meaning that I can run the unit tests locally, they all pass, then create the PR only to discover that there they are failing. If there's a version to restrict (like for the one for typer), that, like it or not, we have to do it from time to time, then we would need to apply that fix in even one more place.

There is already a noted duplication between setup.cfg and environment.yml and I do not think we should have even more.

The environment where the unit tests run is already NOT what it is run in production. Which is fair and fine. But that is why the integrations tests are there.

We expect the CI to run in an environment which is different to out local machines as we intetionally only install the bare minimum in the CI. We also let it use the newest possible version to try and catch issues before they affect the harder/slower to debug tests.

Maybe we want to drop the environment.yaml file entirely but that's a separate discussion to start and maybe we want to use something else to harmonise this but we've not got around to trying it.

for more information, see https://pre-commit.ci

chrisburr · 2024-08-28T12:42:38Z

.pre-commit-config.yaml

@@ -36,3 +36,10 @@ repos:
          - types-aiobotocore[essential]
          - boto3-stubs[essential]
        exclude: ^(diracx-client/src/diracx/client/|diracx-[a-z]+/tests/|diracx-testing/|build)
+
+  - repo: https://github.com/asottile/pyupgrade


We have discussed this before...

#61 (comment)

fstagni · 2024-08-28T19:05:40Z

Superseded by #286

fstagni mentioned this pull request Aug 21, 2024

typer 0.12.4 is creating issues in DIRAC/diracx/diracx-web CI pipelines #280

Closed

chaen reviewed Aug 21, 2024

View reviewed changes

.github/workflows/main.yml Outdated Show resolved Hide resolved

fstagni force-pushed the fixes_looking_around branch 5 times, most recently from dfec561 to 5b30fd0 Compare August 22, 2024 08:29

fstagni force-pushed the fixes_looking_around branch from 5b30fd0 to 60f58b2 Compare August 23, 2024 11:52

chrisburr reviewed Aug 23, 2024

View reviewed changes

.github/workflows/semantic.yml Outdated Show resolved Hide resolved

chrisburr reviewed Aug 23, 2024

View reviewed changes

.github/workflows/main.yml Show resolved Hide resolved

chrisburr reviewed Aug 23, 2024

View reviewed changes

diracx-routers/tests/jobs/test_sandboxes.py Outdated Show resolved Hide resolved

chaen reviewed Aug 23, 2024

View reviewed changes

fstagni force-pushed the fixes_looking_around branch 3 times, most recently from 03480d7 to 992c14e Compare August 26, 2024 11:54

fstagni marked this pull request as ready for review August 26, 2024 12:18

chaen mentioned this pull request Aug 28, 2024

Switch to EdDSA for JWTs in tests #279

Merged

fstagni force-pushed the fixes_looking_around branch from 992c14e to f526a06 Compare August 28, 2024 12:13

fstagni added 5 commits August 28, 2024 14:18

fix: run tests with 1024 bit tokens

9edc80f

test: use environment.yml

0960728

fix: align python version to 3.11

238bad3

test: added pilot tests

196d4e8

fix: added pydantic-settings and opentelemetry

305aac1

fstagni and others added 7 commits August 28, 2024 14:18

fix: avoid deprecation of utcnow

24c4ce4

test: added shellcheck

59db2c3

[pre-commit.ci] auto fixes from pre-commit.com hooks

a264543

for more information, see https://pre-commit.ci

fix: avoid deprecation of utcnow

05b0495

fix: exclude typer versions from 0.12.4

0b65afa

fix: added pyupgrade to pre-commit

147de58

style: pyupgrade --py311-plus

129a37f

fstagni force-pushed the fixes_looking_around branch from f526a06 to 129a37f Compare August 28, 2024 12:18

chrisburr reviewed Aug 28, 2024

View reviewed changes

fstagni closed this Aug 28, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes looking around #281

Fixes looking around #281

fstagni commented Aug 21, 2024

chaen Aug 21, 2024

fstagni Aug 22, 2024

chaen Aug 22, 2024

fstagni Aug 23, 2024 •

edited

Loading

chaen commented Aug 23, 2024

fstagni commented Aug 23, 2024

chrisburr commented Aug 23, 2024 •

edited

Loading

chaen commented Aug 23, 2024

fstagni commented Aug 23, 2024

chrisburr Aug 23, 2024

fstagni Aug 23, 2024

chaen Aug 23, 2024

fstagni Aug 23, 2024

chaen Aug 23, 2024

fstagni Aug 28, 2024

chrisburr Aug 28, 2024

chrisburr Aug 28, 2024

chrisburr Aug 28, 2024

fstagni commented Aug 28, 2024

Fixes looking around #281

Fixes looking around #281

Conversation

fstagni commented Aug 21, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fstagni Aug 23, 2024 • edited Loading

Choose a reason for hiding this comment

chaen commented Aug 23, 2024

fstagni commented Aug 23, 2024

chrisburr commented Aug 23, 2024 • edited Loading

chaen commented Aug 23, 2024

fstagni commented Aug 23, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fstagni commented Aug 28, 2024

fstagni Aug 23, 2024 •

edited

Loading

chrisburr commented Aug 23, 2024 •

edited

Loading