tools/importer-rest-api-specs: refactoring the terraform package to use the standard SDK Types
#4248
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
tombuildsstuff
added
tool/importer-rest-api-specs
tombuildsstuff
force-pushed
the
refactor/importer-rest-api-specs
branch
2 times, most recently
from
be989d4 to
a8ec26a
Compare
manicminer
approved these changes
Jul 2, 2024
| // | ||
| // NOTE: this should only be used within the `terraform` package - with the SDK Model `TerraformResourceDefinition` | ||
| // used elsewhere. | ||
| type WorkInProgressResource struct { |
There was a problem hiding this comment.
Whilst it would necessitate more imports, is it worth having nested internal packages here?
There was a problem hiding this comment.
Yeah I wondered the same here tbh, I ended up deciding against it because the paths felt too long? Particularly when bringing in the API Definitions Parser, which is a bunch of nested directories, but perhaps that's something we fix in a follow-up when the Parser logic is refactored too?
...mporter-rest-api-specs/internal/components/terraform/testing/generate_provider_block_test.go
Outdated
Show resolved
Hide resolved
…to the new SDK Types/a new directory structure Trying to refactor this in-place is a bunch more effort than refactoring the existing logic into a new package, so this temporarily duplicates the files until the entire `terraform` package is refactored, to allow for a cleaner refactor. This also gives the benefit of writing documentation for these as we go.
…into the Schema package, since it's schema related
…kage over to the new structure
… use the SDK Types This is another clean-cut using only the newer SDK types, the older logic will be cleaned up separately in a bit
…e things simpler
…terraform/schema`
…ntification stage This is where the WIP data is built-up, so it might as well start here
…ge over Additional refactoring is needed here, as is documentation - but we need the codepaths re-enabled first
…WIPResource, so that we have this information as needed
…ier flattening The structure may change post-refactoring to simplify this, but it's fine for now
… use the new `terraform` package
…ackage and remnants
tombuildsstuff
force-pushed
the
refactor/importer-rest-api-specs
branch
from
a8ec26a to
327916f
Compare
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:
terraformpackage from./tools/importer-rest-api-specs/components/terraforminto `./tools/importer-rest-api-specs/internal/components/terraform'terraformpackage to both use and return the SDK Models rather than the legacyresourcemanagermodelsIn order to make this work, it's been considerably easier to duplicate the package, refactor it, and then to remove the old one - so the diff for this may be a little strange, but this is ultimately copying, refactoring and then removing the old logic - but fundamentally there's little changed here.
Part 13 of #3754