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

(fix) Handle caFile alone being set for repos #3258

Merged
merged 1 commit into from Apr 12, 2018

Conversation

mparry
Copy link
Contributor

@mparry mparry commented Dec 14, 2017

Previously it was only respected if certFile and keyFile were also
specified. However, these are independent features.

This addresses #3074. (The main motivation was that caFile appears to be a necessary workaround on, for example, MacOS due to golang/go#16532.)

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Dec 14, 2017
Copy link
Member

@bacongobbler bacongobbler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally speaking, we try to retain backwards compatibility in public-facing packages where possible. Is there any way we can retain NewClientTLS and introduce NewTLSConfig as a function that calls NewClientTLS instead?

I'd also add some unit tests to pkg/tlsutil, otherwise this looks good.

@mparry
Copy link
Contributor Author

mparry commented Jan 2, 2018

I have tweaked the change such that NewClientTLS() is reinstated and should work as before. I have also slightly expanded httpgetter_test so that it exercises the new code path at least.

I didn't add any tests directly at the tls level only because there was nothing existing to base them upon, and I didn't want to spend a lot of time inventing some. If you think that the above is still insufficient, I can take another look.

@javefang
Copy link

javefang commented Mar 5, 2018

/LGTM, @bacongobbler what do we need before this can be merged?

Previously it was only respected if certFile and keyFile were also
specified. However, these are independent features.
@mparry
Copy link
Contributor Author

mparry commented Mar 21, 2018

Rebased against master

@nazarewk
Copy link

should also fix #3792 , what is the status of this?

@bacongobbler bacongobbler merged commit e8460a8 into helm:master Apr 12, 2018
splisson pushed a commit to splisson/helm that referenced this pull request Dec 6, 2018
(fix) Handle caFile alone being set for repos
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants