Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
Add remote spaces #12
Conversation
babbageclunk
added some commits
May 5, 2017
axw
approved these changes
May 5, 2017
LGTM. I wonder if we could get away with reusing the existing subnet de/serialisation within remote spaces. Not sure that we'll need any remote-specific bits for subnets?
| @@ -94,7 +94,7 @@ func importRemoteEndpoints(sourceMap map[string]interface{}) ([]*remoteEndpoint, | ||
| } | ||
| func importRemoteEndpointList(sourceList []interface{}, importFunc remoteEndpointDeserializationFunc) ([]*remoteEndpoint, error) { | ||
| - result := make([]*remoteEndpoint, 0, len(sourceList)) | ||
| + var result []*remoteEndpoint |
babbageclunk
May 7, 2017
Member
One of my tests showed that starting with a nil list of endpoints and roundtripping it would result in an empty slice, which seemed a bit nasty. This might be overly finicky. Or I could get the best of both worlds by adding an if sourceList == nil { return nil, nil } at the start.
| + return nil, errors.Annotatef(err, "remote space %d v%d schema check failed", i, version) | ||
| + } | ||
| + valid := coerced.(map[string]interface{}) | ||
| + subnet, err := newRemoteSpaceFromValid(valid, version) |
|
I didn't reuse subnets because I got spooked by the differences between basic subnets and the one we're using:
But I think that was wrong - maybe the right thing to do is to not worry about the extra fields, add a new version for subnet that adds ProviderSpaceId and AvailabilityZones (as a []string), and update the existing import/export code that deals with subnets to work with the updated fields (just putting the single AZ from state into a slice with one element). Does that sound reasonable? From my reading of the release list it looks like the subnets V2 format was introduced in 2.2, which isn't out yet, so I might not need to introduce a new subnet version. I guess it's actually pretty easy to though, so I think I'll do it as V3 just to keep it cleaner. |
Sounds reasonable to me. |
babbageclunk
added some commits
May 8, 2017
babbageclunk
changed the title from
Add remote spaces and remote subnets
to
Add remote spaces
May 8, 2017
|
$$merge$$ |
|
Oh duh. |
babbageclunk commentedMay 5, 2017
These allow adding spaces and bindings to remote applications, which are needed for cross-model relations.