-
Notifications
You must be signed in to change notification settings - Fork 93
Offline flag for v0.12 #73
Offline flag for v0.12 #73
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The logic in the change looks reasonable. One concern I have is that we have members which may not be populated if the value of another member meets a specific condition (eg, if offline is true, resourceManager is nil). I'd feel a bit more comfortable for the long run if the offline vs online behavior was abstracted behind an interface that has an offline and online implementation.
if err := cfg.LoadAndValidate(); err != nil { | ||
return nil, errors.Wrap(err, "configuring") | ||
if !offline { | ||
if err := cfg.LoadAndValidate(); err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it safe to skip the loading step entirely in offline mode?
@@ -122,6 +124,7 @@ type Converter struct { | |||
|
|||
cfg *converter.Config | |||
|
|||
// Talk to GCP resource manager. This field would be nil in offline mode. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should make Converter into an interface and add offline and online implementations.
Overall looks fine, but I concur with @briantkennedy that we should change Converter to an interface with offline/online implementations. |
I'd prefer doing that on a separate pull request (with an issue tracker). How does that sound? |
Ok, opened #75 to track. |
This is based from #72 to add
--offline
flag. It also makes unit test possible without connecting to GCP API.This pull request would invalidate #38 as #38 has no version switch logic.