Refactor writing Terraform resources #14723
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
k8s-ci-robot
added
the
size/L
| elem := v.Index(i) | ||
| if elem.Kind() == reflect.Pointer { | ||
| // TODO can these ever be nil? | ||
| elem = elem.Elem() |
There was a problem hiding this comment.
Agree that panic-ing here probably makes sense. It would be surprising to have a nil in a list of strings...
|
|
||
| func (s *sliceObject) Write(buffer *bytes.Buffer, indent int, key string) { | ||
| for _, member := range s.members { | ||
| member.Write(buffer, indent, key) |
There was a problem hiding this comment.
I was surprised we didn't need [ or , here. But now I see that it's because these are slices of objects, which are just written without delimeters. Makes sense, but a comment might be helpful here.
|
This LGTM, and we do have test coverage. /lgtm /hold to give others a chance to comment, but I think you can self-remove the hold after a few hours @johngmyers |
k8s-ci-robot
added
the
do-not-merge/hold
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: justinsb The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
|
/hold cancel |
k8s-ci-robot
removed
do-not-merge/hold
|
Argh, pushed commit to wrong branch. Removed. |
You sure? |
johngmyers
force-pushed
the
refactor-tf-resource
branch
from
6864a99 to
272171e
Compare
|
I'm having a bad git day, I suppose. |
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
/retest |
This is the big one; a lot of type combinations needed to be covered.
The risk is that there's some combination of types that exists in our code but isn't exercised by our integration tests. I tested a version where all nil pointers were replaced by zero values and all empty slices were replaced by a slice containing a zero value. That didn't produce any panics. So the risk would be a resource type that isn't exercised at all in the integration tests.