GoogleCloudPlatform
fix(tgc): fix resolve image without http client causing panic
b/235263809 Wrap resolve image function to recover from panic if client is nil and return just the original name.
The whole content of the original image.go should go away and instead should use tpg's service/compute/image.go file. However, some files like compute_disk.go is still using private function of resolveImage*.
Release Note Template for Downstream PRs (will be copied)
Hi there, I'm the Modular magician. I've detected the following information about your changes:
Diff report
Your PR generated some diffs in downstreams - here they are.
Terraform GA: Diff ( 1 file changed, 6 insertions(+)) Terraform Beta: Diff ( 1 file changed, 6 insertions(+)) TF Conversion: Diff ( 1 file changed, 6 insertions(+))
Tests analytics
Total tests: 856
Passed tests 791
Skipped tests: 65
Affected tests: 0
Click here to see the affected service packages
- compute
$\textcolor{green}{\textsf{All tests passed in REPLAYING mode.}}$ View the build log
The goal of this work would be to make it so offline users don't get an error in this case. However, +1 regarding your concerns about side effects; this should be a TGC-only change.
What would you think about removing tfplan2cai/converters/google/resources/services/compute/image.go completely and adding in a new TGC-only ResolveImage function that checks if
c.Clientis nil, and returns success if so - and otherwise imports ResolveImage from TPGB and calls that version of the function?
I checked a bit of ResolveImage, it has many paths to validate whether a image path is correct, and only 2 paths use a http client to connect.
To add a new TGC only ResolveImage function will require duplicating quite a bit of code of image.go. (tfplan2cai/converters/google/resources/services/compute/image.go should be removed, since tpgb has the same version of image.go).
I think you could either always return the name from ResolveImage if there's no client:
func ResolveImage(c *transport_tpg.Config, project, name, userAgent string) (string, error) {
if c.Client == nil {
return name
}
return compute.ResolveImage(c, project, name, userAgent)
}
Or you could keep the current method of returning an error instead of panicking (which is good regardless) but add a wrapper in TGC that checks for the right error message(s) that are safe to ignore. For example:
func ResolveImage(c *transport_tpg.Config, project, name, userAgent string) (string, error) {
resolvedName, err := compute.ResolveImage(c, project, name, userAgent)
if err != nil {
if err == "http client is not provided to get image" {
return name, nil
}
return "", err
}
return resolvedName, err
}
I think that you're right that allowing the other methods of resolution would be better.
Hi there, I'm the Modular magician. I've detected the following information about your changes:
Diff report
Your PR generated some diffs in downstreams - here they are.
TF Conversion: Diff ( 3 files changed, 41 insertions(+), 1 deletion(-))
I think you could either always return the name from ResolveImage if there's no client:
func ResolveImage(c *transport_tpg.Config, project, name, userAgent string) (string, error) { if c.Client == nil { return name } return compute.ResolveImage(c, project, name, userAgent) }Or you could keep the current method of returning an error instead of panicking (which is good regardless) but add a wrapper in TGC that checks for the right error message(s) that are safe to ignore. For example:
func ResolveImage(c *transport_tpg.Config, project, name, userAgent string) (string, error) { resolvedName, err := compute.ResolveImage(c, project, name, userAgent) if err != nil { if err == "http client is not provided to get image" { return name, nil } return "", err } return resolvedName, err }I think that you're right that allowing the other methods of resolution would be better.
Done with recovering from panic. However, compute_disk.go is still using ResolveImage, and since it's not a handwritten file, so cannot replace that function.