Change in GOCA behaviour calling `NewConfig`

[peterwillis]

Description The tests are currently failing on the master branch. The behaviour when creating a GOCA Config has changed quite significantly due to this commit. It now performs an os.Exit due to a call to log.Fatalln.

To Reproduce Pass no user or password like this NewConfig(user, password, endpoint string) in an environment where there is no ONE_AUTH environment varaible. This is the case when I run tests that call NewConfig for example.

Expected behavior Not expecting a call to os.Exit, this is a big change. It is not standard practise that go a library would use log.Fatal as it runs os.Exit. One suggestion I have is to add a new function to client.go that can be used by the flow client that works in a different way, and leave the old NewConfig function to behave in the old way.

Details GOCA

Progress Status

• [ ] Code committed
• [ ] Testing - QA
• [ ] Documentation (Release notes - resolved issues, compatibility, known issues)

[peterwillis]

One idea would be to ignore the error like this which would preserve the old behaviour.

[sk4zuzu]

Hi @peterwillis,

We're sorry we broke your tests..

TBH, we still think the previous behavior was incorrect, that's because NewConfig() is not supposed to return errors and since it parses files, to keep any kind of consistency it must fail.

We don't really support such case when both user and password are empty in OpenNebula, in your code you finish up with partially uninitialized struct, that was never our intention (it was a bug). If you need an empty struct you can easily create and fill it yourself, but we are not sure though why such use case should even exist? :thinking: Could you elaborate what you actually do, we are genuinely curious, we'd like to understand more. :pray::slightly_smiling_face:

So for the reference https://github.com/OpenNebula/one/blob/master/src/oca/ruby/opennebula/client.rb#L124-L135 you can see similar logic in Ruby OCA, that's what we actually want. Also the Flow client was calling os.Exit if you look at the commit diff more closely, so we were quite inconsistent between those two. On top of that we don't use this "feature" in tests either in GOCA nor TF provider.

We could introduce something like NewEmptyConfig() and NewEmptyFlowConfig() if you'd like that, I guess. :thinking:

[peterwillis]

Thank you for your response, it's no problem I have added the workaround to our fork for now. The tests are broken here in this repo too by the way.

In our tests we construct the config using empty strings, but override the http client after which enables us to mock the xml-rpc api responses in our tests rather than run a service with a dummy driver. I think I will alter our tests to pass in a non-empty username and password to avoid this issue, so it's not going to cause me any kind of problem for me.

I have some opinions from a Go perspective, but I understand if your goals are different here.

It is not standard practise to use a fatal log which in turn calls os.Exit in a Go library that is designed to be imported into another program. The decision to exit is preferred to be left to the program using the library, as it may want to perform some additional tasks as part of a graceful termination. If the idea intention of NewConfig is to provide a successfully initialized struct but that struct is not valid if there is an error, then it should be also returning an error. That would be the Go equivalent of the code linked in the Ruby client, since it's not advised to exit or panic in those circumstances. If the desired outcome is for the function not to return an error in any circumstances, and it couldn't find those values because the file doesn't exist, then the empty strings are the zero value, which are the same as what you would get in an empty struct. That would mean it's falling back in a reasonable way without returning an error and it's the responsibility of the caller to handle the empty strings. I do see your point that it is probably never desirable to end up with an empty username and password, but I don't think an os.Exit is what I hope to happen here if they are passed, in the same way I wouldn't expect a panic.

If you find that users typically use multiple language clients (Ruby + Go) then I can understand that maybe there is merit to having them behave in a similar way. My perspective is that I only use the Go client and I am hoping for an experience which follows the norms of libraries in that ecosystem rather than how a Ruby client behaves in a quite different runtime with quite different norms that I am not all that familiar with.

Thank you for your time considering this issue :-)