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

Rajiv Asati's Update to plugin_impl_gobgp.go #24

Merged
merged 2 commits into from May 9, 2018

Conversation

rajiva1
Copy link
Contributor

@rajiva1 rajiva1 commented Apr 29, 2018

This is my commit message

Signed-off-by: Rajiv Asati rajiva@cisco.com

Ligato code does NOT work with external .yaml file, because of this:

	func (plugin *Plugin) applyExternalConfig() {
	var externalCfg *config.Bgp
	found, err := plugin.PluginConfig.GetValue(externalCfg)

The above code (line 70) defined externalCfg of pointer type and passed it as-is in PluginConfig.GetValue(externalCfg) in line 71. That meant passing a different memory address from where the external file really was (remember, every time a variable is passed as parameter, a new copy of the variable is created), resulting in a garbage being passed to ParseConfigFromYamlFile function inside p.GetConfigName(), which then returned an error.

Instead, define config.Bgp variable as a pointer to the address where the external .yaml file is, and pass the same pointer in PluginConfig.GetValue(externalCfg) in line 71.
https://gobyexample.com/pointers
http://goinbigdata.com/golang-pass-by-pointer-vs-pass-by-value/

Ligato code does NOT work with external .yaml file, because of this:
//
	func (plugin *Plugin) applyExternalConfig() {
	var externalCfg *config.Bgp
	found, err := plugin.PluginConfig.GetValue(externalCfg)
//
The above code (line 70) defined externalCfg of pointer type and passed it as-is in PluginConfig.GetValue(externalCfg) in line 71. That meant passing a different memory address from where the external file really was (remember, every time a variable is passed as parameter, a new copy of the variable is created), resulting in a garbage being passed to ParseConfigFromYamlFile function inside p.GetConfigName(), which then returned an error.

Instead, define config.Bgp variable as a pointer to the address where the external .yaml file is, and pass the same pointer in PluginConfig.GetValue(externalCfg) in line 71. 
https://gobyexample.com/pointers
http://goinbigdata.com/golang-pass-by-pointer-vs-pass-by-value/
@fgschwan
Copy link
Collaborator

fgschwan commented May 9, 2018

The problem was uninitialized variable externalCfg (not pointing to existing config.Bgp). Changing type from *config.Bgp to config.Bgp is also a possibility, but you forgot to update also second usage of externalCfg. Therefore building fails. Please fix it.
Also please add "Signed-off-by" contributor statement to your commit message (https://github.com/probot/dco#how-it-works) to pass DCO check.

Signed-off-by: Rajiv Asati <rajiva@cisco.com>

Ligato BGP code fails if used with an external .yaml file, because line 70 defined externalCfg of pointer type and passed it as-is in PluginConfig.GetValue() in line 71, as shown below:
//
	func (plugin *Plugin) applyExternalConfig() {
	var externalCfg *config.Bgp
	found, err := plugin.PluginConfig.GetValue(externalCfg)
//
The above meant passing a different memory address from where the external file really was (remember, every time a variable is passed as parameter, a new copy of the variable is created), resulting in a garbage being passed to ParseConfigFromYamlFile() function inside p.GetConfigName(), which then returned an error.

Fix is to define config.Bgp variable as a pointer to the address where the external .yaml file is, and pass the same pointer in PluginConfig.GetValue() in line 71 and 80. 

https://gobyexample.com/pointers
http://goinbigdata.com/golang-pass-by-pointer-vs-pass-by-value/
@rajiva1
Copy link
Contributor Author

rajiva1 commented May 9, 2018

Hi Filip, thanks for your review. I have updated the changes.
I am not sure why DCO failed earlier, since "signed-off-by:" contributor statement was included in the commit message. Hopefully, it has gone away.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 81.633% when pulling 34f1a50 on rajiva1:patch-1 into 7fc98a9 on ligato:master.

@fgschwan
Copy link
Collaborator

fgschwan commented May 9, 2018

I found out that the DCO is checking every commit, so it will fail because the first commit doesn't have the "signed-off-by". But i think it is ok because the last commit has it.

@fgschwan fgschwan merged commit 782b500 into ligato:master May 9, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants