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

notifier: add ServerName configuration for TLS #47

Merged
merged 2 commits into from
Dec 15, 2015
Merged

notifier: add ServerName configuration for TLS #47

merged 2 commits into from
Dec 15, 2015

Conversation

jzelinskie
Copy link
Contributor

No description provided.

@Quentin-M
Copy link
Contributor

You changed the generic utility function loadTLSClientConfig to make it specific to the HTTP Notifier. True it is not used by anything else at the moment, but it may be. Also, almost none of the functions in the utility package are re-used at that point (ie. LoadTLSClientConfigForServer, the sibling function is just used in the API as well) - but they are pretty generic imho and could be re-used at some point.

@jzelinskie
Copy link
Contributor Author

True it is not used by anything else at the moment, but it may be.

This is a good reason why we should wait and see if it can be reused before refactoring it out. If it can, refactor it out; if it can't, keep it local to the thing using it. The reason I made it localized was because if we continue adding new options for the TLS config, the parameters to the function could get huge. To handle this problem you can do a number of things:

  • Make the function specific to *config.NotifierConfig
  • Make the function specific to a new *config.TLSConfig
  • Make the function take a *tls.Config instead of allocating our own inside the function

I chose the least-invasive option.

@Quentin-M
Copy link
Contributor

Sounds good to me.
Should we do the same thing with LoadTLSClientConfigForServer then?

This isn't reused any where just yet, so we're best off leaving it local
to the place that needs it.
@Quentin-M
Copy link
Contributor

LGTM

Quentin-M pushed a commit that referenced this pull request Dec 15, 2015
notifier: add ServerName configuration for TLS
@Quentin-M Quentin-M merged commit 172693b into master Dec 15, 2015
@Quentin-M Quentin-M deleted the sn branch December 15, 2015 19:32
@jzelinskie jzelinskie added area/usability related to improving user experience kind/feature request wishes for new functionality/docs reviewed/lgtm labels Mar 12, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/usability related to improving user experience kind/feature request wishes for new functionality/docs
Development

Successfully merging this pull request may close these issues.

None yet

2 participants