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 dependencies and now compatible from netstandard2.0 to net8.0 #166

Merged
merged 1 commit into from
Nov 4, 2024

Conversation

energywave
Copy link
Contributor

  • Updated all NuGet dependencies
  • reverded the base target from netstandard2.1 (introduced in ver 1.2.7) to netstandard2.0 as this keeps it compatible with the still widely used Net Framework 4.8.
  • Added net4.8 to Tests targets and adapted appsettings.json code to work on every platform (used newtonsoft.json to read it instead of the AspNetCore configuration).
  • In DistanceMatrixTests.ShouldReplaceUriViaOnUriCreated test implemented the placeholder replacement without using the UriBuilder as it was buggy in .net 4.8
    All tests passed on all targets.
    I'm already using in one of my .net 4.8 WinForms project and it's working flawlessy.

I've updated all dependencies and reverded the base target from netstandard2.1 (introduced in ver 1.2.7) to netstandard2.0 as this keeps it compatible with the widely used Net Framework 4.8.
Added net4.8 to tests and adapted appsettings.json code to work on every platform.
All tests passed on all targets.
@maximn
Copy link
Owner

maximn commented Oct 31, 2024

@energywave thanks for the PR!
The tests are failing, can you have a look? I'll also try to have a look tmrw. Seems like something with your change of the API KEY.

The way it works, there is an environment variable named GOOGLE_API_KEY with the key that's used in the tests.

@energywave
Copy link
Contributor Author

Hello, yes I've seen it was trying to get the key from app settings.json and, if not available there, it was getting from the environment variable.
I've changed only the appsettings part, leaving as it was the environment part.
In fact on my PC I've used the key in appsettings as I've seen you excluded that file in .gitignore (nice!) and in my visual studio all tests in all the targets are completing flawlessly.
I'll try to figure out what is happening tomorrow, I hope to find time.
Thank you for the useful library! ☺️

@maximn
Copy link
Owner

maximn commented Nov 2, 2024

Another suggestion is to split this PR to smaller chunks, so it'll be easier to merge it

@energywave
Copy link
Contributor Author

Sorry that I could not work on that sooner. When I submitted my PR I was using the appsettings to set my google apy key so I had the doubt that setting the environment variable did not work. So I tested it yesterday and... it just works correctly!
So, if you download my PR and you test it in Visual Studio on Windows it works just as intended and all tests in all three targets are passed.
There must be something in github actions but I never used them... Can you please check that they works correctly? As I suspect that even without my PR they'll not pass...
Specifically the previouse code was:

protected string ApiKey => Configuration.GetValue<string>(ApiKeyEnvironmentVariable) 
?? Environment.GetEnvironmentVariable(ApiKeyEnvironmentVariable) 
?? throw new InvalidOperationException($"API key is not configured. Please set the {ApiKeyEnvironmentVariable} environment variable.");

And now is:

protected string ApiKey => AppSettings.Load()?.GoogleApiKey
?? Environment.GetEnvironmentVariable(ApiKeyEnvironmentVariable) 
?? throw new InvalidOperationException($"API key is not configured. Please set the {ApiKeyEnvironmentVariable} environment variable.");

As you see the environment should work. The only reason because it would not pick up the environment variable is if it finds an appsettings and it contains the GOOGLE_APY_KEY variable.

@energywave
Copy link
Contributor Author

Furthermore... I think we have to update the nuget.yml adding other targets accordint to .net standard 2.0 but, again, I never used github workflow, so maybe it's better that you fix it the proper way.

@maximn
Copy link
Owner

maximn commented Nov 4, 2024

I think that something with the jobs for PRs isn't set right and you're just not getting the key. I'll check that.

@maximn maximn merged commit 359626b into maximn:master Nov 4, 2024
1 of 2 checks passed
@maximn
Copy link
Owner

maximn commented Nov 4, 2024

Published v 1.3.3

@energywave
Copy link
Contributor Author

Wonderful, thank you! I've updated on of my softwares where I was referencing my PR in the solution, by now referencing the newly published package from NuGet and it works flawlessy in .net framework 4.8.
Maybe when you have time check the workflow on PR for the future integrations :)
And thank you for your library!

@maximn
Copy link
Owner

maximn commented Nov 4, 2024

Yeah, I checked the github actions documents. Looks like I need to clone your PR to a local branch and then run it there. I'll do that for next time.

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