Start experimental preview command #4438
Conversation
justinsb
force-pushed
the
cli_preview
branch
2 times, most recently
from
e3cc422 to
b2731e0
Compare
justinsb
changed the title
|
/assign @cheftako |
justinsb
force-pushed
the
cli_preview
branch
5 times, most recently
from
aa5f131 to
42c3db8
Compare
| func (e BlockedGCPError) Error() string { | ||
| // encode in json so that we can unwrap even if this is (incorrectly) wrapped as a string (terraform) | ||
| j, _ := json.Marshal(e) | ||
| return fmt.Sprintf("call to GCP blocked (method=%v, url=%v) [jsonstart:BlockedGCPError:%v:jsonend]", e.Method, e.URL, string(j)) |
There was a problem hiding this comment.
Why %v if we know they are strings?
There was a problem hiding this comment.
Honestly because %v works for both, but %s does not.
| // GRPCUnaryClientInterceptor intercepts GCP GRPC API calls. | ||
| func (c *interceptingGCPClient) GRPCUnaryClientInterceptor() grpc.UnaryClientInterceptor { | ||
| return func(ctx context.Context, method string, req, reply any, cc *grpc.ClientConn, invoker grpc.UnaryInvoker, opts ...grpc.CallOption) error { | ||
| // TODO! |
There was a problem hiding this comment.
Do we want this commented out block?
There was a problem hiding this comment.
We're going to need it to support GRPC-based resources. It's a placeholder and reminder :-)
| } | ||
|
|
||
| func (c *interceptingControllerRuntimeClient) SubResource(subResource string) client.SubResourceClient { | ||
| panic("not implemented") |
There was a problem hiding this comment.
Are we sure we want to panic? I buy that we need to exit the current resource being tested. However it would be nice to get info on more that just the first problem we hit?
There was a problem hiding this comment.
Changed it to not panic! In general we don't actually use most of these methods, I believe. So I'd rather figure out how to catch the panics (I think we do). But switched this one to not panic (it's pretty similar to status above).
pkg/cli/preview/preview_test.go
Outdated
| }() | ||
|
|
||
| // TODO: Add real waiting | ||
| time.Sleep(time.Second * 10) |
There was a problem hiding this comment.
If we want to wait for recorder.objects > 0 with timeout, I think we have helper functions which could do that. Seems like that would average faster and be less flaky.
There was a problem hiding this comment.
Agreed, but I think it's pretty involved if we want to package this as a CLI tool, and I'd rather leave this task for a follow-on PR. (It's easy to do it for this test, because we know how many objects there are, but I'd rather fix it for the arbitrary case)
There was a problem hiding this comment.
Added the proposed wait for objects > 0. There might still be a race here though I think it is unlikely. But we're eventually going to want a reconcileEnd event, I think.
| } | ||
|
|
||
| // recordDiff captures the diff into our recorder. | ||
| func (r *Recorder) recordDiff(ctx context.Context, diff *structuredreporting.Diff) { |
There was a problem hiding this comment.
Slight concern this is being done on the primary goroutine of the reconcile. My concern is what happens if we have large volumes of data and a slow disk. Also what happens if the disk runs out of room?
There was a problem hiding this comment.
So even in "preview mode" it only goes to memory. In existing modes of operation we expect no listeners.
I will add a warning to the interface though, saying you should treat this as a logger and generally copy the data and queue it for another thread if you are going to do anything substantial with it (like writing to disk)
The goal of the command is to capture for each object the changes that will be made at the cloud level. This step adds some of the mechanics and a test, it does not yet expose the functionality.
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cheftako 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 |
google-oss-prow
bot
merged commit 8543b37
into
GoogleCloudPlatform:master
The goal of the command is to capture for each object the changes that will be made at the cloud level.
This step adds some of the mechanics and a test, it does not yet expose the functionality.