sts-sponsors team mailing list archive

[Alberto Donato <mp+443281@xxxxxxxxxxxxxxxxxx>]

nice, looks mostly good.

A few minor comments inline

Diff comments:

> diff --git a/backend/msm/db/models.py b/backend/msm/db/models.py
> index 078946a..edabb41 100644
> --- a/backend/msm/db/models.py
> +++ b/backend/msm/db/models.py
> @@ -38,6 +45,7 @@ class Site(BaseModel):
>      street: str | None
>      timezone: TimeZone | None
>      url: str
> +    connection_status: str

this should be a ConnectionStatus

>      stats: SiteData | None
>  
>  
> diff --git a/backend/msm/db/queries.py b/backend/msm/db/queries.py
> index 77b173e..16df66f 100644
> --- a/backend/msm/db/queries.py
> +++ b/backend/msm/db/queries.py
> @@ -163,6 +168,19 @@ async def get_sites(
>              case(
>                  (
>                      SiteData.c.site_id != None,  # noqa: E711
> +                    case(
> +                        (
> +                            SiteData.c.last_seen > connection_lost_timedelta,
> +                            ConnectionStatus.STABLE,
> +                        ),
> +                        else_=ConnectionStatus.LOST,
> +                    ),
> +                ),
> +                else_=ConnectionStatus.UNKNOWN,
> +            ).label("connection_status"),

couldn't this be a single case?

case(
   (
      SiteData.c.site_id == None,
      ConnectionStatus.UNKNOWN,
   ),
   (
      SiteData.c.last_seen > connection_lost_timedelta,
      ConnectionStatus.STABLE,
   ),
   else_=ConnectionStatus.LOST,
)

> +            case(
> +                (
> +                    SiteData.c.site_id != None,  # noqa: E711
>                      func.json_build_object(
>                          "total_machines",
>                          (
> diff --git a/backend/msm/settings.py b/backend/msm/settings.py
> index a010b54..4bfb651 100644
> --- a/backend/msm/settings.py
> +++ b/backend/msm/settings.py
> @@ -44,5 +44,9 @@ class Settings(BaseSettings):
>  
>      access_token_expire_minutes = int(getenv("TOKEN_EXPIRATION_TIME", 30))
>  
> +    lost_connection_threshold_seconds = int(
> +        getenv("LOST_CONNECTION_THRESHOLD", 60)
> +    )
> +

please prefix all env vars with MSM_ (TOKEN_EXPIRATION_TIME should also be prefixed)

>  
>  SETTINGS = Settings()


-- 
https://code.launchpad.net/~r00ta/maas-site-manager/+git/site-manager/+merge/443281
Your team MAAS Committers is requested to review the proposed merge of ~r00ta/maas-site-manager:MAASENG-1628 into maas-site-manager:main.