documentation: documenting the current architecture/workflow and summarising the proposed changes to the Data API going forward #3425
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…anges proposed for Data API v2 This PR documents the current architecture/workflow - but also documents/discusses the proposed changes for Data API v2 (around having the Data API become the sole owner of the API as we've previously discussed). I've also split the `how automation works` section of the README out into the docs folder, for easier grouping
stephybun
requested changes
Dec 6, 2023
There was a problem hiding this comment.
Thanks @tombuildsstuff for the diagrams. Left some suggestions which would be good to have looked at.
Comment on lines
+42
to
+47
| The following guides are internal/technical documentation - documenting the overall architecture and requiring additional context: | ||
|
|
||
| * [Internals: Architecture Diagrams](internals-architecture-diagrams.md) | ||
| * [Internals: How the Automation Works](internals-how-automation-works.md) | ||
| * [Internals: Future - changes to the Data API/Architecture](internals-architectural-changes-data-api.md) | ||
|
|
There was a problem hiding this comment.
As suggested over in #3266 I think these should be under a different header and not ## Guides
Comment on lines
+38
to
+53
| Whilst it was possible to coordinate changes to both Importers and the Data API for the API Definitions, given that we | ||
| may need to support additional Importers in the future (e.g. Data Plane) and that in order for each Importer to support | ||
| Identifying and Building Terraform Resources in the future - it's likely that divergences would emerge over time and that | ||
| there's value in having a greater separation of concerns here. | ||
|
|
||
| As such there's a number of improvements we can make to the system design which'll make life easier going forwards, | ||
| namely having the Data API owning the API Definitions - and splitting out a dedicated tool for identifying and building | ||
| the Terraform Data Sources and Resources from the API Definitions. | ||
|
|
||
| This approach allows each tool to have a single focus - becoming far simpler and thus easier to reason with - which will | ||
| enable reducing the complexity of the `importer-rest-api-specs` tool in particular. | ||
|
|
||
| To do this we'll want to make two changes to the current system design: | ||
|
|
||
| 1. Have the Data API become the sole maintainer of the API Definitions. | ||
| 2. Split out the Terraform Identification/Builder logic from the `importer-rest-api-specs` tool. |
There was a problem hiding this comment.
You're repeating yourself in several different ways here, this can and should be expressed more succinctly.
Suggested change
| Whilst it was possible to coordinate changes to both Importers and the Data API for the API Definitions, given that we | |
| may need to support additional Importers in the future (e.g. Data Plane) and that in order for each Importer to support | |
| Identifying and Building Terraform Resources in the future - it's likely that divergences would emerge over time and that | |
| there's value in having a greater separation of concerns here. | |
| As such there's a number of improvements we can make to the system design which'll make life easier going forwards, | |
| namely having the Data API owning the API Definitions - and splitting out a dedicated tool for identifying and building | |
| the Terraform Data Sources and Resources from the API Definitions. | |
| This approach allows each tool to have a single focus - becoming far simpler and thus easier to reason with - which will | |
| enable reducing the complexity of the `importer-rest-api-specs` tool in particular. | |
| To do this we'll want to make two changes to the current system design: | |
| 1. Have the Data API become the sole maintainer of the API Definitions. | |
| 2. Split out the Terraform Identification/Builder logic from the `importer-rest-api-specs` tool. | |
| Currently an Importer has the following responsibilities: | |
| * Parsing incoming data from a service type e.g. Resource Manager, Microsoft Graph and potentially Data Plane in the future | |
| * Outputting API definitions for the `go-azure-sdk` | |
| * Identifying and building Terraform resources | |
| Divergences within the different service types may emerge over time, which would result in additional overhead and complexity to the existing design. As a result there's value in having a greater separation of concerns here with the goal being a singular focus for each tool, thus reducing the complexity of the Importer, i.e. `importer-rest-api-specs`. | |
| To do this we'll want to make two changes to the current system design: | |
| 1. Have the Data API become the sole maintainer of the API Definitions. | |
| 2. Split out the Terraform Identification/Builder logic from the `importer-rest-api-specs` tool. |
Comment on lines
+105
to
+106
| Whilst the files used for the API Definitions are in sync, we can gradually migrate from the current setup to publishing | ||
| the API Definitions to the Data API. |
There was a problem hiding this comment.
I don't understand how the files being in sync allows us to gradually migrate.
Comment on lines
+105
to
+116
| Whilst the files used for the API Definitions are in sync, we can gradually migrate from the current setup to publishing | ||
| the API Definitions to the Data API. | ||
|
|
||
| This means that we'll need to have a Data API instance available in order to run each Importer, which whilst might | ||
| sound not ideal - means that we could extend the Data API to better support local development in the future. | ||
| For example, supporting launching the Data API with a feature-flag where the API Definitions are stored in-memory | ||
| only - meaning that these don't need to be persisted to disk, speeding up local development - or other changes which | ||
| may seem helpful in the future. | ||
|
|
||
| In order to run this in automation, we'll need to add a couple of new modes to the `wrapper-automation` tool, to support | ||
| launching the Data API prior to running each Importer, but that should be mostly reusable from the existing logic / a | ||
| previous implementation as can be found [in this pull request](https://github.com/hashicorp/pandora/pull/1637). |
There was a problem hiding this comment.
This feels very much like specific implementation detail which is information that should go into an issue rather than here.
Comment on lines
+125
to
+127
| Rather than doing this we should introduce a separate tool, working name `Terraform Resource Builder` - which focuses | ||
| solely on Building Terraform Resources (that is, the Terraform Schema, any Mappings and the Acceptance Tests) based | ||
| on the data within the Data API. |
There was a problem hiding this comment.
You already clarified what building a terraform resource entails, this is unnecessary repetition
Suggested change
| Rather than doing this we should introduce a separate tool, working name `Terraform Resource Builder` - which focuses | |
| solely on Building Terraform Resources (that is, the Terraform Schema, any Mappings and the Acceptance Tests) based | |
| on the data within the Data API. | |
| Rather than doing this we should introduce a separate tool, working name `Terraform Resource Builder`, which focuses | |
| solely on Building Terraform Resources based on the data within the Data API. |
Comment on lines
+129
to
+131
| This allows each Importer (that is, `importer-rest-api-specs` and `importer-msgraph-metadata`) to remain focused on | ||
| Importing the API Definitions - where the API Definitions are then pushed into the Data API - with the Terraform Resource | ||
| Builder pulling information from the Data API - and subsequently pushing any changes back to it. |
There was a problem hiding this comment.
Context has been set on what importers we have and are referring to, this is also repetition
Suggested change
| This allows each Importer (that is, `importer-rest-api-specs` and `importer-msgraph-metadata`) to remain focused on | |
| Importing the API Definitions - where the API Definitions are then pushed into the Data API - with the Terraform Resource | |
| Builder pulling information from the Data API - and subsequently pushing any changes back to it. | |
| This allows each Importer to remain focused on | |
| Importing the API Definitions - where the API Definitions are then pushed into the Data API - with the Terraform Resource | |
| Builder pulling information from the Data API - and subsequently pushing any changes back to it. |
| Importing the API Definitions - where the API Definitions are then pushed into the Data API - with the Terraform Resource | ||
| Builder pulling information from the Data API - and subsequently pushing any changes back to it. | ||
|
|
||
| Whilst spreading these tools out does introduce a little more overhead, it makes the process boundaries clearer - since |
There was a problem hiding this comment.
Suggested change
| Whilst spreading these tools out does introduce a little more overhead, it makes the process boundaries clearer - since | |
| Whilst spreading these tools out does introduce a little more overhead, it makes the process boundaries clearer, since |
|
|
||
| Whilst spreading these tools out does introduce a little more overhead, it makes the process boundaries clearer - since | ||
| the inputs and outputs for each tool are more clearly defined. For example, this should make supporting Data Sources in | ||
| the future simpler - since we can first add support the Terraform Resource Builder, push that data into the Data API, |
There was a problem hiding this comment.
I don't know what this is trying to say first add support the Terraform Resource Builder add or support? or add support for what?
Suggested change
| the future simpler - since we can first add support the Terraform Resource Builder, push that data into the Data API, | |
| the future simpler, since we can first add support the Terraform Resource Builder, push that data into the Data API, |
|
Closing this out for now |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR documents the current architecture/workflow - but also documents/discusses the proposed changes for Data API v2 (around having the Data API become the sole owner of the API as we've previously discussed).
I've also split the
how automation workssection of the README out into the docs folder, for easier grouping