search

alphagov / notifications-net-client

.NET client for the GOV.UK Notify API
https://www.nuget.org/packages/GovukNotify/
MIT License
25 stars 20 forks source link

Updated dos and added JWT version range 7-10 #173

Open samuelhwilliams opened 1 year ago

samuelhwilliams commented 1 year ago

Internal copy of https://github.com/alphagov/notifications-net-client/pull/171

What problem does the pull request solve?

Checklist

samuelhwilliams commented 1 year ago

just validating this with jwt==10 as that's failing at the moment 🤔

RocktimeLtd commented 1 year ago

jwt10 is deprecated so shouldn't be used 10.0.2 is fine

samuelhwilliams commented 1 year ago

Not sure if maybe my attempt at testing this is wrong?

I have manually edited src/GovukNotify/GovukNotify.csproj to set JWT range to [10.0.2,11) in an attempt to get it to build+test with v10.0.2, and on building I get:

  Determining projects to restore...
/var/project/src/GovukNotify.Tests/GovukNotify.Tests.csproj : error NU1605: Detected package downgrade: Newtonsoft.Json from 13.0.2 to 10.0.3. Reference the package directly from the project to select a different version.  [/var/project/GovukNotify.sln]
/var/project/src/GovukNotify.Tests/GovukNotify.Tests.csproj : error NU1605:  GovukNotify.Tests -> GovukNotify -> JWT 10.0.2 -> Newtonsoft.Json (>= 13.0.2)  [/var/project/GovukNotify.sln]
/var/project/src/GovukNotify.Tests/GovukNotify.Tests.csproj : error NU1605:  GovukNotify.Tests -> GovukNotify -> Newtonsoft.Json (>= 10.0.3 && < 14.0.0) [/var/project/GovukNotify.sln]
/var/project/src/GovukNotify/GovukNotify.csproj : error NU1605: Detected package downgrade: Newtonsoft.Json from 13.0.2 to 10.0.3. Reference the package directly from the project to select a different version.  [/var/project/GovukNotify.sln]
/var/project/src/GovukNotify/GovukNotify.csproj : error NU1605:  GovukNotify -> JWT 10.0.2 -> Newtonsoft.Json (>= 13.0.2)  [/var/project/GovukNotify.sln]
/var/project/src/GovukNotify/GovukNotify.csproj : error NU1605:  GovukNotify -> Newtonsoft.Json (>= 10.0.3 && < 14.0.0) [/var/project/GovukNotify.sln]

Any thoughts? When I read that it seems to say to me that it wants to use NewtonSoft.Json == 10.0.3 rather than 13.0.2 but I'm not sure why.

RocktimeLtd commented 1 year ago

The support for package ranges is a bit odd. for testing i had to select the ones i wanted to test against from nuget manager. Then build that version. if you aren't using VS set the versions in the project file first

samuelhwilliams commented 1 year ago

Are you able to confirm that the client will install for users on JWT==10.0.2?

As I don't use .net/VS, my process is:

1) Apply this patch:

    diff --git a/src/GovukNotify/GovukNotify.csproj b/src/GovukNotify/GovukNotify.csproj
    index 48be67c..88e5f44 100644
    --- a/src/GovukNotify/GovukNotify.csproj
    +++ b/src/GovukNotify/GovukNotify.csproj
    @@ -16,7 +16,7 @@
       </PropertyGroup>

       <ItemGroup>
    -    <PackageReference Include="JWT" Version="[7.1,11)" />
    +    <PackageReference Include="JWT" Version="[10.0.2,11)" />
         <PackageReference Include="Newtonsoft.Json" Version="[10.0.3,14)" />
         <PackageReference Include="System.Collections.Specialized" Version="4.3.0" />
         <PackageReference Include="System.Reflection" Version="4.3.0" />

2) Run make bootstrap-with-docker test-with-docker 3) See the dependency errors above, indicating failure to match+install dependencies.

I don't feel confident merging this at the moment as I'm not sure that the client can work with v10.0.2 of JWT?

RocktimeLtd commented 1 year ago

Just wondering how you are getting on?

RocktimeLtd commented 8 months ago

I have run the unit tests and the local ones all pass. I need the following test information to run the other tests against the live api.

NOTIFY_API_URL API_KEY API_SENDING_KEY EMAIL_TEMPLATE_ID SMS_TEMPLATE_ID LETTER_TEMPLATE_ID EMAIL_REPLY_TO_ID SMS_SENDER_ID INBOUND_SMS_QUERY_KEY

Regards kieron