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

Follow-up from #943 #945

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Follow-up from #943 #945

wants to merge 1 commit into from

Conversation

mheese
Copy link
Contributor

@mheese mheese commented Jan 15, 2020

Follow-up from #943 . Adding better logging for http client certs and adding stops in envoy.

There could be a lot more done in both packages that are touched here. However, especially the http package stands out: logging is not very clear, UT missing (in the works though for envoy by Abhi), a lot of variable/pointer checks missing, unclear return values and missing errors - should at least have a comment if they are intended, ignoring golang best practices and not very elegant code. One can see that this was written down in a hurry, and fixed in a hurry by us recently as well :) This package deserves some love.

Anyway, I leave it up to @brianonn and @abhijitherekar to add more to this PR if you think that is necessary.

@brianonn I ignored the Gopkg.toml comment from #943 as I don't think we need to change anything. Please comment here if you think otherwise.

Copy link
Contributor

@brianonn brianonn left a comment

Choose a reason for hiding this comment

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

Nice! thanks @mheese . I like the way you made your changes.

I still think we should pin the Gopkg.toml entry for conntrack and not let it float around with master.

[[constraint]]
  name = "github.com/ti-mo/conntrack"
  revision = "c9b176489c1a057922cbaa0a78df9e51cffbcc0d"

Is there a reason why you think this is not necessary? I never like choosing a branch tip and I always like to have it pinned to a commit and reproducible, rather than allow the version we pull in to change (and possibly open it to new bugs or security holes that are introduced)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants