search

gophercloud / utils

Apache License 2.0
20 stars 55 forks source link

Expose the cloud YAML file location #92

Closed fabianofranz closed 5 years ago

fabianofranz commented 5 years ago

Cloud configs can be loaded from several files (cloud.yaml, clouds-public.yaml, secure.yaml) in a number of different locations (user's config dir, host config dir, current dir, a dir specified by an env var), and there's no way for users of the clientconfig packages to know which one(s) were actually used after calling GetCloudFromYAML.

This PR introduces the ability to call it by providing a callback function that receives the full path to the file(s) loaded. An example use case would be to log the path as info or debug log levels inside the callback function, so that end users could use it for debugging purposes.

No API changes, all existing exported functions were kept as is.

fabianofranz commented 5 years ago

@jtopjian PTAL

jtopjian commented 5 years ago

@fabianofranz Thank you for submitting this.

I understand the use-case and you raise a really good point. Taking this a step further, if someone wanted to do further customizations, then additional callbacks would be required. So I've broke this down into two main ideas:

  1. The filename of the YAML file is hidden and not exposed in any way.
  2. There is no way of overriding / customizing the way YAML files are loaded.

I created a solution here which should work: https://github.com/gophercloud/utils/compare/master...jtopjian:loadyaml-refactor

There's a unit test that provides an example of how this can be used. And since the other unit tests pass, this confirms this change is also backwards compatible.

There is one caveat: I have not provided a way to override the GetCloudFromYAML or AuthOptions functions. There is way too much logic and internal/private functions being used to make sense to allow someone to override, IMO.

Let me know what you think and/or if you have any questions.

fabianofranz commented 5 years ago

@jtopjian right, customizing the YAML loading logic would be my next need, so thanks for doing this! I believe that file loading is the place where most use cases for customization would exist, so no problem with leaving GetCloudFromYAML out.

LGTM

jtopjian commented 5 years ago

@fabianofranz Sounds good. I've merged #93 with this functionality. I'm going to close this PR but please let me know if you have any questions.

Thanks again for raising this topic!