Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

GCE: Handle a nil configuration file #1121

Merged
merged 1 commit into from
Aug 16, 2017

Conversation

nicksardo
Copy link
Contributor

@nicksardo nicksardo commented Aug 12, 2017

Fixes #1112

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Aug 12, 2017
@k8s-reviewable
Copy link

This change is Reviewable

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.06%) to 44.614% when pulling bc5881b on nicksardo:gce-nil-config into 1516493 on kubernetes:master.

return bytes.NewReader(allConfig)
}
} else {
glog.V(2).Infoln("Not using cloudprovider config file")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it expected to work without config?
If not, we should return an error here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should "work" without config assuming defaults are sane enough for the user.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

optional nit: Update the message to say "No config file provided. Continuing with default values".
Config file not provided is different than not using config file.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

}
glog.V(2).Infof("Using cloudprovider config file:\n%v ", string(allConfig))

getConfigReader = func() io.Reader {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of redefining getConfigReader, you can just update the variable that it returns.

configReader := nil
getConfigReader := func() io.Reader { return configReader }
if config != nil {
 // Update configReader
}

Copy link
Contributor Author

@nicksardo nicksardo Aug 15, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not quite that simple. If we went this route, configReader would need to be reset for each loop iteration.

for {
   if config != nil {
       configReader = bytes.NewReader(allConfig)
   }
...
...
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

aah, got it.
Sorry for missing it the first time

@nikhiljindal
Copy link
Contributor

thx, lgtm with an optional log message update suggestion

@nicksardo nicksardo added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 15, 2017
@nicksardo nicksardo merged commit 7891ed6 into kubernetes:master Aug 16, 2017
@nicksardo nicksardo deleted the gce-nil-config branch August 16, 2017 00:00
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.06%) to 44.725% when pulling df65973 on nicksardo:gce-nil-config into 81a36b4 on kubernetes:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.06%) to 44.725% when pulling df65973 on nicksardo:gce-nil-config into 81a36b4 on kubernetes:master.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants