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

[Feature Contribution]: Remove JSON examples from WebAPI container #440

Open
2 tasks done
YaSuenag opened this issue Dec 22, 2023 · 8 comments
Open
2 tasks done

[Feature Contribution]: Remove JSON examples from WebAPI container #440

YaSuenag opened this issue Dec 22, 2023 · 8 comments
Assignees

Comments

@YaSuenag
Copy link
Member

What happened?

I'd like to propose to remove following JSON files from WebAPI container:

  • /app/data-sources/json/test-data-azure-emissions.json
  • /app/data-sources/json/test-data-aws-emissions.json
  • /app/location-sources/json/azure-regions.json
  • /app/appsettings.json

They are example files to try WebAPI easily, I think. But they are not necessary in the container use case because container user should set their account for backend services (e.g. Electricity Maps, WattTime) and location config. If they exist in container, the user might not be aware soon their wrong when some configurations are missed.

Code of Conduct

  • I agree to follow this project's Code of Conduct

Feature Commitment

  • I commit to contributing this feature as a PR and working with the GSF to merge this feature into the Carbon Aware SDK.
@jacksorjacksor
Copy link
Contributor

As a user who's experimenting with implementing POC instances of Carbon Aware SDK I feel the sample data should be there to confirm first-instance usage (i.e. I want to focus on deploying and ensuring I can get a result, once I've done that I can change the data source to be WattTime/Electricity Maps).

So I personally wouldn't want the sample data to be removed.

@YaSuenag
Copy link
Member Author

YaSuenag commented Jan 5, 2024

AFAIK RDBMS container (e.g. PostgreSQL) does not contain sample data. So it is nature not to contain them in WebAPI container IMHO.

If you want to confirm your deployment status, you can use healthcheck endpoint (/health).
If you want to try to use the SDK, you should configure with sample data, and it should be documented (maybe it is better to be described into quickstart.md, but it has been not yet.

I agree that sample data is useful to try to the SDK, so they should keep in the repo. I just want to say they should be removed from WebAPI container.

@jacksorjacksor
Copy link
Contributor

@YaSuenag fair enough! Thanks for the clarification.

@YaSuenag
Copy link
Member Author

YaSuenag commented Jan 7, 2024

Further investigation, I think it is better to do following things rather than just remove JSON files from WebAPI container. I will send PR if we can make agreement.

About appsettings.json, we need to remove following lines:

"DataSources": {
"EmissionsDataSource": "test-json",
"ForecastDataSource": "", // We don't currently publish a sample test data source for forecasts.
"Configurations": {
"test-json": {
"Type": "JSON",
"DataFileLocation": "test-data-azure-emissions.json"
}
}
},

In addirtion, it might be better to remove both location JSON and emission data JSON from packaging item:

<Content Include="$(TargetDir)/data-sources/**/*.json" Pack="true" PackagePath="data-sources/" />
<Content Include="$(TargetDir)/location-sources/**/*.json" Pack="true"

However some files depend on test-data-azure-emissions.json - I think we should remove these dependencies.

private const string DefaultDataFile = "test-data-azure-emissions.json";

"CarbonAwareStaticJsonDataService": {
"data-file": "data-files/test-data-azure-emissions.json"
},

YaSuenag added a commit to YaSuenag/carbon-aware-sdk that referenced this issue Jan 21, 2024
Copy link
Contributor

github-actions bot commented May 7, 2024

This issue has not had any activity in 120 days. Please review this issue and ensure it is still relevant. If no more activity is detected on this issue for the next 20 days, it will be closed automatically.

@github-actions github-actions bot added the stale label May 7, 2024
@YaSuenag
Copy link
Member Author

YaSuenag commented May 7, 2024

Wait! I've sent PR for this issue: #450

Copy link
Contributor

github-actions bot commented Sep 5, 2024

This issue has not had any activity in 120 days. Please review this issue and ensure it is still relevant. If no more activity is detected on this issue for the next 20 days, it will be closed automatically.

@github-actions github-actions bot added the stale label Sep 5, 2024
@YaSuenag
Copy link
Member Author

YaSuenag commented Sep 5, 2024

This issue does not stale!

I've sent PR #450 for this issue, and I will fix conflictions when other PRs are merged.

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

No branches or pull requests

4 participants