Skip to content
This repository has been archived by the owner on May 15, 2023. It is now read-only.

extract ancestry discovery logic into an interface #82

Merged
merged 2 commits into from Oct 4, 2019

Conversation

yukinying
Copy link
Contributor

@yukinying yukinying commented Oct 3, 2019

This fixes #75 with a small change of scope. Instead of making converter an interface, this creates a new interface called AncestryManager which has a GetAncestry method.

The implementation for online mode uses resource manager and a cache. This works the same way as when --offline=false.

Test has been added with a mock of resource manager API server for GetAncestry call.

Also, the need of calling converter.LoadAndValidate() is removed as the upstream dependency (terraform_google_conversion) do not requires the clients to be constructed for the TF module supported. Please review that as well.

I would also like to get help to see if the names for the interface and methods make sense. I cannot make up a better name.

@yukinying
Copy link
Contributor Author

Another reason I want to abstract the ancestry discovery logic as an interface is that there are more than one way to find ancestry. E.g. if you have access to CAI, you could probably find the ancestry data there (correct me if I am wrong). In that sense, I would not need to enable resource manager API.

@morgante
Copy link
Contributor

morgante commented Oct 3, 2019

This definitely looks like a change in a good direction!

I think I'd still prefer that offline and online mode be represented by two different types/structs (which both implement the ancestryManager interface). Otherwise every access to resource manager will need a nil check (or potentially introduce runtime errors).

@yukinying
Copy link
Contributor Author

I think I'd still prefer that offline and online mode be represented by two different types/structs (which both implement the ancestryManager interface). Otherwise every access to resource manager will need a nil check (or potentially introduce runtime errors).

I have thought about it for a while. In any case we need to perform nil check before calling am.resourceManager.Projects.GetAncestry(...), so offline and online ancestry manager implementation is pretty much the same.

@morgante
Copy link
Contributor

morgante commented Oct 3, 2019

I have thought about it for a while. In any case we need to perform nil check before calling am.resourceManager.Projects.GetAncestry(...), so offline and online ancestry manager implementation is pretty much the same.

Why do we need the nil check in both cases?

In offline mode, we should simply never be making that call.
In online mode, we need a resource manager to initialize the ancestryManager.

@yukinying
Copy link
Contributor Author

True. I have made the changes.

ancestrymanager/ancestrymanager.go Outdated Show resolved Hide resolved
@morgante morgante merged commit 1206372 into GoogleCloudPlatform:master Oct 4, 2019
@yukinying yukinying deleted the interface branch November 27, 2019 00:57
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make offline mode a separate type
2 participants