Green-Software-Foundation/carbon-aware-sdk

[ADR Review]: Review of ADRs

Closed this issue · 7 comments

Willmish commented

What happened?

This is to track the review of the following ADRs:

These ADRs are to be reviewed independently but discussion can be on this thread. If members have discussions via Zoom/Teams/etc outputs from those discussions must be put in this thread as they relate to the reviews.

Code of Conduct

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

Worth adding here the previously lead discussions under the PRs for proposing these ADRs:

  • Dynamic Data Source Registration: #288
  • Add option to display CLI output in CSV format #310

@bderusha feel free to reiterate your thoughts here for increased visability.

YaSuenag commented

I think ADR-0014 (Dynamic Data Souce Registration) relates to #345 .

#345 propose to support multiple data source in config JSON, but I think it affects to ADR-0014 because current implementation handles data source as a singleton. I think it is better to discuss more to reorganize related ADRs.

  • 👍1
bderusha commented

ADR-0015
I would prefer this ADR to be more opinionated before it is approved or denied.

  • Either to propose a 3rd party CSV dependency or outline requirements for building the functionality needed to support this new feature.
  • For forecasting data format I think that having a row for each flattened ForecastData value and a column for each flattened OptimalDataPoint value is a more user-friendly format. Specifically, putting the OptimalDataPoints at the end so that it's easier to manage them since the first 10 columns will always be the same, and the remaining columns can be parsed programmatically. EG:
RequestedAt Location ... ForecastDataTime ForecastDataRating OptimalTime_0 OptimalRating_0 OptimalTime_1 OptimalRating_1
2023-04-01T00:00:00Z EastUS ... 2023-04-01T01:00:00Z 123 2023-04-01T01:00:00Z 123 2023-04-01T02:00:00Z 123
2023-04-01T00:00:00Z EastUS ... 2023-04-01T02:00:00Z 123 2023-04-01T01:00:00Z 123 2023-04-01T02:00:00Z 123
2023-04-01T00:00:00Z EastUS ... 2023-04-01T03:00:00Z 456 2023-04-01T01:00:00Z 123 2023-04-01T02:00:00Z 123

NIT: The ADR file is 0015, but the ADR text heading is 13. The value in numbering is more about ordering the files in the directory, I am not sure of the value in having a number in the heading.

bderusha commented

ADR-0014

I think that this is an important evolution for the project and that we should get this ADR to a place where we are comfortable accepting it. For me the changes required would be:

  • Update the "context" section to reflect the current state of the project and the challenges with that state (not to propose the solution)
  • Update the "decision" section to reflect the decision that solves that challenge (AKA the code supporting the use of external data sources)
  • Update the "consequences" section to reflect what that means for the project: The benefits listed in the current "decision" section and the costs, such as maintaining multiple packages and dependency management, especially for interface changes.
  • Update "design considerations" to highlight and discuss only the most meaningful changes:
    • Making interfaces and data records consumable by external packages.
    • Potential changes to how dependency injection is managed
    • NOTE: the rest are either changes without significant consequences (like making a data source packable) or covered by existing ADRs (like how to dynamically load a data source, covered by ADR-6)
Willmish commented

Decision during meeting #355 - postponing decision by one week giving people more time to review

github-actions commented

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 commented

This issue has not had any activity for too long. If you believe this issue has been closed in error, please contact an administrator to re-open, or if absolutly relevant and necessary, create a new issue referencing this one.