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

Watt Time v3 Support #532

Merged
merged 34 commits into from
Jun 28, 2024
Merged

Watt Time v3 Support #532

merged 34 commits into from
Jun 28, 2024

Conversation

vaughanknight
Copy link
Contributor

Pull Request

Issue Number: #510

Summary

PR for all WattTime v3 updates

Changes

  • TBC

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.

Are there API Changes?

If yes, what are the expected API Changes? Please link to an API-Comparison
workflow with the API Diff.

Is this a breaking change?

If yes, what workflow does this break?

Anything else?

Other comments, collaborators, etc.

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

This PR Closes #<issue_number>

First draft of the ADR for watt time v3 changes.  Looking at path mappings and parameters.  Still plenty to work on.
Moved ADR to correct location
updated notes for BA

Signed-off-by: Vaughan Knight <[email protected]>
First draft of the ADR for watt time v3 changes.  Looking at path mappings and parameters.  Still plenty to work on.
Moved ADR to correct location
updated notes for BA

Signed-off-by: Vaughan Knight <[email protected]>
Added AuthenticationBaseUrl to the configuration and updated Authentication to leverage the v3 authenication path - note the API is not updated and will require further updates.
Updated start and end configuration to new values
Balancing Authority Renamed to Region.  Does not include updates to API, just the Query String parameter.
Updates for historical data API
Removed accidental file
Lots of test updates, need to do some fixes.
Historical forecasts updated
I, Vaughan Knight <[email protected]>, hereby add my Signed-off-by to this commit: b9490e4
I, Vaughan Knight <[email protected]>, hereby add my Signed-off-by to this commit: b991bac
I, Vaughan Knight <[email protected]>, hereby add my Signed-off-by to this commit: e4f1494
I, Vaughan Knight <[email protected]>, hereby add my Signed-off-by to this commit: b443e9e
I, Vaughan Knight <[email protected]>, hereby add my Signed-off-by to this commit: ab1205d
I, Vaughan Knight <[email protected]>, hereby add my Signed-off-by to this commit: 7c115fa
I, Vaughan Knight <[email protected]>, hereby add my Signed-off-by to this commit: e047c9a
I, Vaughan Knight <[email protected]>, hereby add my Signed-off-by to this commit: aa81382
I, Vaughan Knight <[email protected]>, hereby add my Signed-off-by to this commit: 8640c8c
I, Vaughan Knight <[email protected]>, hereby add my Signed-off-by to this commit: 880fcf7

Signed-off-by: Vaughan Knight <[email protected]>
Many tests reworked, a few to go.  Consolidated a lot of the hand crafted json objects into objects that get serialized as the purist JsonObject format was prone to errors - in some cases tests were passing even with bad typing.
Further test updates
Further updates, just 1 test left to remediate
…ests

Updated to add authentication client to the service builder for the tests.  All tests now passing.
Renaming of Balancing Authority to Region through all code and comments.  This will also need updating through documentation.
Fixed spelling error in latitude
Updates based on code review for WattTime Tests.  Mostly cleanup of constants which were removed elsewhere in tests.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These updates will need propagating to the version upgrade documentation @danuw just tagging you so we don't forget. RegionCacheTTL replaces BalancingAuthorityCacheTTL.

Cleaned up a lot of the string literals.  They were causing too much fragility in the code base, and made it complex when updating the WattTime API.
More cleanup of some of the strings.  Creating consistency for using the test data on parameters and not just reponses also.
Updates to documentation and changelog
@Willmish
Copy link
Collaborator

Willmish commented Jun 25, 2024

EDIT: False alarm, the below tests succeed on a fresh clone of the repo, the below stack trace I am leaving for openness’s sake, but it was due to a remnant appsettings.Development.json file sitting in CarbonAware.WebApi/src.

The failing test suite:
carbon-aware-sdk/src/CarbonAware.WebApi/test/integrationTests
Stack trace from failed integration test (Ran by dotnet test in /src folder on a development docker container):
Trace here: https://pastebin.com/LPvpZQkZ

@Willmish Willmish self-requested a review June 25, 2024 08:00
Copy link
Collaborator

@Willmish Willmish left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, I left 3 comments for small changes, which would be nice to see but happy for it to get merged without them as well. Thanks for moving WattTime test magic numbers to a single file btw :)

@@ -238,7 +249,8 @@ private async Task UpdateAuthTokenAsync()
private void SetBasicAuthenticationHeader()
{
var authToken = Encoding.UTF8.GetBytes($"{this._configuration.Username}:{this._configuration.Password}");
this._client.DefaultRequestHeaders.Authorization = new AuthenticationHeaderValue(AuthenticationHeaderTypes.Basic, Convert.ToBase64String(authToken));
//this._client.DefaultRequestHeaders.Authorization = new AuthenticationHeaderValue(AuthenticationHeaderTypes.Basic, Convert.ToBase64String(authToken));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can drop the comment? (this._client will not exist after the PR comes through in this context)



[JsonPropertyName("meta")]
public GridEmissionsMetaData Meta { get; set; }
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: constructor will leave null value if called without params, maybe make nullable (add ?) or return empty.



[JsonPropertyName("meta")]
public GridEmissionsMetaData Meta { get; set; }
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NIT: same as before, can make nullable or by default initialise an empty GridEmissionsMetaData object

Copy link
Contributor

@YaSuenag YaSuenag left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both ElectricityMaps and ElectricityMapsFree datasources works fine on my machine, and also all tests has passed on Visual Studio 2022 Community.

Copy link
Collaborator

@danuw danuw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

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

Successfully merging this pull request may close these issues.

4 participants