Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
Ensure cloud.Name is set and used everywhere #6884
Conversation
| @@ -81,14 +84,16 @@ func (s *CloudSuite) TestAddCloudDuplicate(c *gc.C) { | ||
| } | ||
| func (s *CloudSuite) TestAddCloudNoType(c *gc.C) { | ||
| - err := s.State.AddCloud("stratus", cloud.Cloud{ | ||
| + err := s.State.AddCloud(cloud.Cloud{ | ||
| + Name: "stratus", | ||
| AuthTypes: cloud.AuthTypes{cloud.AccessKeyAuthType, cloud.UserPassAuthType}, | ||
| }) | ||
| c.Assert(err, gc.ErrorMatches, `invalid cloud: empty Type not valid`) |
babbageclunk
approved these changes
Jan 30, 2017
Sorry for not doing this when I added Cloud.Name.
If we're relying on the key in clouds.yaml taking precedence over cloud name in the case that they're different, could you add a test to ensure that?
| @@ -156,6 +156,7 @@ type cloudSet struct { | ||
| // cloud is equivalent to Cloud, for marshalling and unmarshalling. | ||
| type cloud struct { | ||
| + Name string `yaml:"name"` |
babbageclunk
Jan 30, 2017
Member
We're serialising out the cloud name here now - does that mean that we'll do the wrong thing for users that have exisiting yaml files without the name attribute in clouds? I guess it'll be ok as long as the reading still code overrides the name from the key. Although that does make it possible for the key and the name to be inconsistent in the file (which is why I didn't change this when I added name). Is that a problem? It might be confusing.
axw
Jan 30, 2017
Member
Ah, actually that's a mistake - we should only be serialising the name outside of the clouds.yaml file format. i.e. clouds.yaml should continue to be formatted as:
name:
type: foo
...and for other cases of YAML serialisation, we'll get:
name: foo
type: bar
...I have updated the code and added tests.
axw
Jan 30, 2017
Member
Also, I'll add a test to show that we ignore the name field in the YAML file if it's there.
|
$$merge$$ |
|
Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju |
|
Build failed: Tests failed |
|
$$merge$$ |
|
Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju |
axw commentedJan 30, 2017
A recent change introduced the Name field to
cloud.Cloud. In several parts of the code
base we were not setting it properly. This
branch ensures that it is always filled in,
introduces additional validation in state,
and updates various bits of code to stop
passing around the cloud name separately.
QA steps