Using common transport for https based writes #103
Conversation
This commit creates one common create transport function for https based writes, rather than having duplicating code. In addition, the timeouts for these transports are created in defaults.yaml so they can be changed easily. Other changes include: 1. Adding TLSSkipVerify to client's director query, which is a useful feature for testing for us when using a running director locally. 2. Found a bug in namespaces.go, where NamespaceURL was used when TopologyNamespaceURL should be used instead.
There was a typo in main_test.go where it was setting NamespaceURL and not TopologyNamespaceURL like it should. Bug is now fixed and tests seem to pass (at least locally).
More fixes to pass unit tests. Seems like namespaces_test.go was also setting NamespaceURL instead of TopologyNamespaceURL so fixed this. Also added in config.go a way to set topologynamespaces from environment variables to make these tests behave how they should.
|
@joereuss12 - can you take a look at the linter failures? Also, the |
config/config.go
Outdated
| @@ -393,6 +393,12 @@ func InitClient() error { | |||
| break | |||
| } | |||
| } | |||
| for _, prefix := range prefixes { | |||
| if val, isSet := os.LookupEnv(prefix + "_TOPOLOGYNAMESPACEURL"); isSet { | |||
There was a problem hiding this comment.
Let's follow the pattern above - prefix + _TOPOLOGY_NAMESPACE_URL:
| if val, isSet := os.LookupEnv(prefix + "_TOPOLOGYNAMESPACEURL"); isSet { | |
| if val, isSet := os.LookupEnv(prefix + "_TOPOLOGY_NAMESPACE_URL"); isSet { |
There was a problem hiding this comment.
Fixed (making note for myself)
There was a problem hiding this comment.
@bbockelm Should we think about making use of the AutomaticEnv features in viper, which I think is already in use elsewhere? From the docs:
AutomaticEnv is a powerful helper especially when combined with SetEnvPrefix.
When called, Viper will check for an environment variable any time a viper.Get
request is made. It will apply the following rules. It will check for an environment
variable with a name matching the key uppercased and prefixed with the EnvPrefix
if set.
This allows us to do something like viper.Get("TopologyNamespaceUrl"), and viper will first check if the environment variable <PELICAN/OSDF/STASH>_TOPOLOGYNAMESPACEURL is set, and if not it will then read/default to whatever is set for the actual string in viper. I think this would allow us to skip manually checking for various env vars like we do in this block.
I also set up a feature here to map nested config structs like Director.DefaultResponse to be configurable by env vars where the period is replaced with _, eg viper.GetString("Director.DefaultResponse") will first check the environment variable PELICAN_DIRECTOR_DEFAULTRESPONSE and if nothing is set, it will use whatever the configured value is.
I think this simplifies configuring environment variables, albeit at the expense of making those variables uglier and harder to read.
There was a problem hiding this comment.
Yeah -- for anything new, we should use definitely use viper.Get(...) as we import the environment variables as well.
This is mostly for backward compatibility: AutomaticEnv only works with a single prefix and for namespaces we've supported a list of prefixes.
In other words, we shouldn't create any more of these -- but I'm ok with this one for now.
First commit to fix issues with PR. Adjusted some formatting in handle_http for the linter to be happy. Also changed the config and tests to follow pattern of _TOPOLOGY_NAMESPACE_URL instead of _TOPOLOGYNAMESPACEURL from prior commit.
Made transport a global variable to only set it up once to improve efficiency and in docs say should only be created once. Also moved Justin's additions to direcotry for "TLSSkipVerify" and added that to handle_http.go with the creation of the transport to make code more concise.
Code now should (hopefully) pass all unit tests, they pass locally. Also changed some formatting the linter was yelling about.
This commit creates one common create transport function for https based writes, rather than having duplicating code. In addition, the timeouts for these transports are created in defaults.yaml so they can be changed easily.
Other changes include: