[terraform-conversions] Add compute_instance to conversions library #1496
Conversation
|
We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google. ℹ️ Googlers: Go here for more info. |
|
Hello! I am a robot who works on Magic Modules PRs. I have detected that you are a community contributor, so your PR will be assigned to someone with a commit-bit on this repo for initial review. They will authorize it to run through our CI pipeline, which will generate downstream PRs. Thanks for your contribution! A human will be with you soon. |
There was a problem hiding this comment.
The PR as is will break terraform which you'll need to fix.
As a larger pattern though you are copying whole files from third_party/terraform and moving them into third_party/validator. Some of them are copied across without modification and others are just changing the method signature. Copying files like this incurs a ton of technical debt because any modification upstream will not propagate to your tool. It also appears that you will not be using many of the methods and just adding copy/pasted dead code into your downstream codebase.
Any files that are copied wholesale should just be sourced from the original location instead of copied by hand into third_party/validator.
For the method signature changes, it would be preferable if you can use those expanders as is. Are you able to build a shim to convert between the Terraform object and your TerraformResourceData object?
| computeBeta "google.golang.org/api/compute/v0.beta" | ||
| ) | ||
|
|
||
| func expandAliasIpRanges(ranges []interface{}) []*computeBeta.AliasIpRange { |
There was a problem hiding this comment.
This looks like it's a copy of the original compute_instance_helpers with the flatten helpers stripped out and a couple lines of validation removed. IMO the validation logic isn't really needed in Terraform either and I think the long term burden of this tool will get more complex with every file that you have to fork. So I propose 2 improvements:
-
If you are importing this just to remove the validation logic but would otherwise be able to use the file as is I'll submit a PR upstream and remove that logic (once I've made sure the api is also validating this in a sane way)
-
If fix philosphy typo #1 doesn't work can you document in a top of file comment what is going on in this file? Something as simple as what the delta is and why. This will save a lot of time in the future when these methods need to be updated upstream.
There was a problem hiding this comment.
I am now just using the file from third_party/terraform. Do you want to remove the validation check in this PR or in another?
|
Unfortunately we are unable to use |
|
Removed copying of files from |
| } | ||
|
|
||
| // Create the instance information | ||
| return &computeBeta.Instance{ |
There was a problem hiding this comment.
We use the computeBeta library for legacy reasons. If you find that this becomes a problem you can probably move to the GA version.
|
Hi! I'm the modular magician, I work on Magic Modules. Pull request statusesNo diff detected in Ansible. New Pull RequestsI built this PR into one or more new PRs on other repositories, and when those are closed, this PR will also be merged and closed. |
|
Signed CLA |
|
CLAs look good, thanks! ℹ️ Googlers: Go here for more info. |
Tracked submodules are build/terraform-beta build/terraform-mapper build/terraform build/ansible build/inspec.
modular-magician
force-pushed
the
cai-compute-instance
branch
from
681c381
to
9e57474
Compare
Add non-generated compute_instance to CAI conversions. Code was pulled from terraform provider
third_party/and modified slightly.