Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fixes looking around #281

Closed
wants to merge 12 commits into from
Closed

Fixes looking around #281

Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
9edc80f
fix: run tests with 1024 bit tokens
fstagni Aug 16, 2024
0960728
test: use environment.yml
fstagni Aug 16, 2024
238bad3
fix: align python version to 3.11
fstagni Jul 5, 2024
196d4e8
test: added pilot tests
fstagni Jul 5, 2024
305aac1
fix: added pydantic-settings and opentelemetry
fstagni Aug 15, 2024
24c4ce4
fix: avoid deprecation of utcnow
fstagni Aug 16, 2024
59db2c3
test: added shellcheck
fstagni Aug 16, 2024
a264543
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] Aug 21, 2024
05b0495
fix: avoid deprecation of utcnow
fstagni Aug 16, 2024
0b65afa
fix: exclude typer versions from 0.12.4
fstagni Aug 21, 2024
147de58
fix: added pyupgrade to pre-commit
fstagni Aug 23, 2024
129a37f
style: pyupgrade --py311-plus
fstagni Aug 23, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .github/workflows/integration.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,16 +46,21 @@ jobs:
run: cd /tmp/DIRACRepo && ./integration_tests.py install-server
- name: Install client
run: cd /tmp/DIRACRepo && ./integration_tests.py install-client
- name: Install pilot
run: cd /tmp/DIRACRepo && ./integration_tests.py install-pilot
- name: Server tests
run: cd /tmp/DIRACRepo && ./integration_tests.py test-server || touch server-tests-failed
- name: Client tests
run: cd /tmp/DIRACRepo && ./integration_tests.py test-client || touch client-tests-failed
- name: Pilot tests
run: cd /tmp/DIRACRepo && ./integration_tests.py test-pilot || touch pilot-tests-failed
- name: Check test status
run: |
has_error=0
# TODO: set has_error=1 when we are ready to really run the tests
if [ -f server-tests-failed ]; then has_error=0; echo "Server tests failed"; fi
if [ -f client-tests-failed ]; then has_error=0; echo "Client tests failed"; fi
if [ -f pilot-tests-failed ]; then has_error=0; echo "Pilot tests failed"; fi
if [ ${has_error} = 1 ]; then exit 1; fi
- name: diracx logs
if: ${{ failure() }}
Expand Down
23 changes: 16 additions & 7 deletions .github/workflows/main.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,21 @@ defaults:
shell: bash -el {0}

jobs:

shellcheck:
fstagni marked this conversation as resolved.
Show resolved Hide resolved
runs-on: ubuntu-latest
if: github.event_name != 'push' || github.repository == 'DIRACGrid/diracx'
timeout-minutes: 30

steps:
- uses: actions/checkout@v4
- name: Run shellcheck
# Excluded codes related to sourcing files
# SC1090: Can't follow non-constant source
# SC1091: Not following sourced file
run: |
find -name '*.sh' -print0 | xargs -0 -n1 shellcheck --exclude=SC1090,SC1091 --external-source

pytest:
name: Unit test - ${{ matrix.package }}
runs-on: ubuntu-latest
Expand All @@ -39,13 +54,7 @@ jobs:
- uses: mamba-org/setup-micromamba@v1
with:
# TODO: Use a conda environment file used for the diracx/base container image
environment-name: test-env
create-args: >-
python=3.11
m2crypto
python-gfal2
mypy
pip
environment-file: environment.yml
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

init-shell: bash
post-cleanup: 'all'
- name: Set up environment
Expand Down
15 changes: 11 additions & 4 deletions .pre-commit-config.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
# See https://pre-commit.com for more information
# See https://pre-commit.com/hooks.html for more hooks
default_language_version:
python: python3
python: python3.11

repos:
- repo: https://github.com/pre-commit/pre-commit-hooks
Expand All @@ -13,18 +13,18 @@ repos:
- id: check-added-large-files

- repo: https://github.com/astral-sh/ruff-pre-commit
rev: 'v0.5.0'
rev: v0.6.2
hooks:
- id: ruff
args: ["--fix"]

- repo: https://github.com/psf/black
rev: 24.4.2
rev: 24.8.0
hooks:
- id: black

- repo: https://github.com/pre-commit/mirrors-mypy
rev: v1.10.1
rev: v1.11.1
hooks:
- id: mypy
additional_dependencies:
Expand All @@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have discussed this before...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#61 (comment)

rev: v3.17.0
hooks:
- id: pyupgrade
args: ["--py311-plus"]
exclude: ^(diracx-client/src/diracx/client/|diracx-cli/tests/|diracx-cli/src/diracx/cli/)
2 changes: 1 addition & 1 deletion diracx-api/pyproject.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
name = "diracx-api"
description = "TODO"
readme = "README.md"
requires-python = ">=3.10"
requires-python = ">=3.11"
keywords = []
license = {text = "GPL-3.0-only"}
classifiers = [
Expand Down
4 changes: 2 additions & 2 deletions diracx-cli/pyproject.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
name = "diracx-cli"
description = "TODO"
readme = "README.md"
requires-python = ">=3.10"
requires-python = ">=3.11"
keywords = []
license = {text = "GPL-3.0-only"}
classifiers = [
Expand All @@ -20,7 +20,7 @@ dependencies = [
"gitpython",
"pydantic",
"rich",
"typer",
"typer <0.12.4",
"pyyaml",
]
dynamic = ["version"]
Expand Down
2 changes: 1 addition & 1 deletion diracx-client/pyproject.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
name = "diracx-client"
description = "TODO"
readme = "README.md"
requires-python = ">=3.10"
requires-python = ">=3.11"
keywords = []
license = {text = "GPL-3.0-only"}
classifiers = [
Expand Down
2 changes: 1 addition & 1 deletion diracx-core/pyproject.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
name = "diracx-core"
description = "Common code used by all DiracX packages"
readme = "README.md"
requires-python = ">=3.10"
requires-python = ">=3.11"
keywords = []
license = {text = "GPL-3.0-only"}
classifiers = [
Expand Down
3 changes: 1 addition & 2 deletions diracx-core/tests/test_secrets.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this PR? Can we leave for another one?

)
private_key_pem = private_key.private_bytes(
encoding=serialization.Encoding.PEM,
Expand Down
2 changes: 1 addition & 1 deletion diracx-db/pyproject.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
name = "diracx-db"
description = "TODO"
readme = "README.md"
requires-python = ">=3.10"
requires-python = ">=3.11"
keywords = []
license = {text = "GPL-3.0-only"}
classifiers = [
Expand Down
2 changes: 1 addition & 1 deletion diracx-routers/pyproject.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
name = "diracx-routers"
description = "TODO"
readme = "README.md"
requires-python = ">=3.10"
requires-python = ">=3.11"
keywords = []
license = { text = "GPL-3.0-only" }
classifiers = [
Expand Down
3 changes: 1 addition & 2 deletions diracx-routers/tests/auth/test_standard.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -527,8 +527,7 @@ async def test_refresh_token_invalid(test_client, auth_httpx_mock: HTTPXMock):
# Encode it differently (using another algorithm)
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,
)
pem = private_key.private_bytes(
encoding=serialization.Encoding.PEM,
Expand Down
2 changes: 1 addition & 1 deletion diracx-routers/tests/test_job_manager.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@fstagni fstagni Aug 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

date = datetime.now(tz=timezone.utc).isoformat(sep=" ").split("+")[0]
r = normal_user_client.patch(
f"/api/jobs/{valid_job_id}/status",
json={
Expand Down
2 changes: 1 addition & 1 deletion diracx-testing/pyproject.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
name = "diracx-testing"
description = "TODO"
readme = "README.md"
requires-python = ">=3.10"
requires-python = ">=3.11"
keywords = []
license = {text = "GPL-3.0-only"}
classifiers = [
Expand Down
11 changes: 10 additions & 1 deletion environment.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ channels:
- conda-forge
- nodefaults
dependencies:
- python =3.11
- authlib
- aiohttp
- aiomysql
Expand Down Expand Up @@ -31,7 +32,13 @@ dependencies:
- isodate
- mypy
- opensearch-py
- opentelemetry-api
- opentelemetry-exporter-otlp
- opentelemetry-instrumentation-fastapi
- opentelemetry-instrumentation-logging
- pre-commit
- pydantic >=2.4
- pydantic-settings
- pyjwt
- pytest
- pytest-asyncio
Expand All @@ -45,7 +52,9 @@ dependencies:
- requests
- rich
- sqlalchemy
- typer
- shellcheck
# Exclude version from 0.12.4 because of https://github.com/DIRACGrid/diracx/issues/280
- typer <0.12.4
- types-cachetools
- types-PyYAML
- types-requests
Expand Down
2 changes: 1 addition & 1 deletion pyproject.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
name = "diracx"
description = "Client installation for users of DiracX installations"
readme = "README.md"
requires-python = ">=3.10"
requires-python = ">=3.11"
keywords = []
license = { text = "GPL-3.0-only" }
classifiers = [
Expand Down
5 changes: 3 additions & 2 deletions run_local.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,8 @@ export DIRACX_DB_URL_SANDBOXMETADATADB="sqlite+aiosqlite:///:memory:"
export DIRACX_DB_URL_TASKQUEUEDB="sqlite+aiosqlite:///:memory:"
export DIRACX_SERVICE_AUTH_TOKEN_KEY="file://${signing_key}"
export DIRACX_SERVICE_AUTH_STATE_KEY="${state_key}"
export DIRACX_SERVICE_AUTH_ALLOWED_REDIRECTS='["http://'$(hostname| tr -s '[:upper:]' '[:lower:]')':8000/docs/oauth2-redirect"]'
hostname_lower=$(hostname | tr -s '[:upper:]' '[:lower:]')
export DIRACX_SERVICE_AUTH_ALLOWED_REDIRECTS='["http://'"$hostname_lower"':8000/docs/oauth2-redirect"]'
export DIRACX_SANDBOX_STORE_BUCKET_NAME=sandboxes
export DIRACX_SANDBOX_STORE_AUTO_CREATE_BUCKET=true
export DIRACX_SANDBOX_STORE_S3_CLIENT_KWARGS='{"endpoint_url": "http://localhost:3000", "aws_access_key_id": "console", "aws_secret_access_key": "console123"}'
Expand All @@ -45,7 +46,7 @@ uvicorn --factory diracx.routers:create_app --reload &
diracx_pid=$!

success=0
for i in {1..10}; do
for _ in {1..10}; do
if curl --silent --head http://localhost:8000 > /dev/null; then
success=1
break
Expand Down