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

Emissions-forecasts with ElectricityMaps will return server error 500 #365

Merged
merged 1 commit into from
Aug 21, 2023

Conversation

xcbinder
Copy link
Contributor

@xcbinder xcbinder commented Jun 14, 2023

Pull Request

Issue Number: #364

Summary

ElectricityMaps forecast scenario will return server error 500.
Repro using CLI: "emissions-forecasts -l westeurope -v"

public async Task GetCurrentCarbonIntensityForecastAsync(Location location) is causing the EM server to return 500. Reason is that the "." in geolocation.Latitude will be passed as ","

Changes

public async Task<EmissionsForecast> GetCurrentCarbonIntensityForecastAsync(Location location)

  1. Fixing Input parameter validation for second condition to Longitude:

if (geolocation.Latitude != null && geolocation.Longitude != null)

  1. Changing parameter parsing to use geolocation.LatitudeAsCultureInvariantString() and geolocation.LongitudeAsCultureInvariantString() to keep the correct formatting of the values.

GetForecastedCarbonIntensityAsync(geolocation.LatitudeAsCultureInvariantString(), geolocation.LongitudeAsCultureInvariantString());

Checklist

  • Local Tests Passing?
  • CICD and Pipeline Tests Passing?
  • Added any new Tests?
  • Documentation Updates Made?
  • Are there any API Changes? If yes, please describe below.
  • This is not a breaking change. If it is, please describe it below.

Is this a breaking change?

If yes, what workflow does this break?

Please follow
GitHub's suggested syntax
to link Pull Requests to Issues via keywords

This PR Closes #364

Issue : 

I have tested a ElectricityMaps forecast scenario CLI: 
"emissions-forecasts -l westeurope -v"
will return server error 500.

Analysis:

public async Task<EmissionsForecast> GetCurrentCarbonIntensityForecastAsync(Location location) is causing the EM server to return 500. Reason is that the "." in geolocation.Latitude will be passed as "," 

Fix:

The following changes are fixing this bug.

Signed-off-by: Christian Binder <[email protected]>
@xcbinder xcbinder requested a review from vaughanknight as a code owner June 14, 2023 16:19
@bderusha bderusha added the Ready for Review PR Ready for review with the GSF team for merge label Jun 29, 2023
@bderusha
Copy link
Contributor

@vaughanknight @Willmish Can you kick off the tests?

@Willmish
Copy link
Collaborator

Willmish commented Jul 4, 2023

#367 meeting update: @vaughanknight to connect with PR owner and discuss further

@danuw
Copy link
Collaborator

danuw commented Aug 21, 2023

Changes have been approved by @bderusha and @FabioTurati-NTT - moving this on.

@danuw danuw merged commit 371905a into Green-Software-Foundation:dev Aug 21, 2023
@danuw danuw self-assigned this Aug 21, 2023
@danuw danuw removed the Ready for Review PR Ready for review with the GSF team for merge label Mar 5, 2024
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.

[Bug]: ElectricityMaps emissions-forecasts will return server error 500
5 participants