[ADR Review]: Review of ADRs

[Willmish]

What happened?

This is to track the review of the following ADRs:

• Dynamic Data Source Registration https://github.com/Green-Software-Foundation/carbon-aware-sdk/blob/dev/docs/architecture/decisions/0014-dynamic-datasource-registration.md (related to yet unimplemented: https://github.com/Green-Software-Foundation/carbon-aware-sdk/blob/dev/docs/architecture/decisions/0006-data-source-registration.md)
• Add option to display CLI output in CSV format https://github.com/Green-Software-Foundation/carbon-aware-sdk/blob/dev/docs/architecture/decisions/0015-cli-output-csv.md

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]

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

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

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

[YaSuenag]

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.

[bderusha]

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]

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]

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

[github-actions]

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]

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.