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.

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

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

samuelhwilliams
Copy link
Contributor

Internal copy of #171

What problem does the pull request solve?

Checklist

  • I’ve used the pull request template
  • I’ve written unit tests for these changes
  • I’ve update the documentation (in DOCUMENATION.md and CHANGELOG.md)
  • I’ve bumped the version number (in src/GovukNotify/GovukNotify.csproj)

@samuelhwilliams
Copy link
Contributor Author

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

@RocktimeLtd
Copy link

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

@samuelhwilliams
Copy link
Contributor Author

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
Copy link

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
Copy link
Contributor Author

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
Copy link

Just wondering how you are getting on?

@RocktimeLtd
Copy link

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants