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

Code for the juju http wrapper package. #1

Merged
merged 1 commit into from
Jul 28, 2020
Merged

Conversation

hmlanigan
Copy link
Member

This code has been moved from juju/utils with a few differences: there is one constructer which takes a config, including the ability to pass a logger; it does not cache proxy settings; and uses httptrace on http.Get methods when trace logging enabled.

http_test.go Outdated Show resolved Hide resolved
Makefile Outdated
# Reformat source files.
format:
## format: Format the go source code
gofmt -w -l .
Copy link
Member

Choose a reason for hiding this comment

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

nit: new line at end of file.

Copy link
Member Author

Choose a reason for hiding this comment

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

done

// a Client with a locally constructed Transport via NewHttpTLSTransport
// and init() will no longer be needed.
func init() {
defaultTransport := http.DefaultTransport.(*http.Transport)
Copy link
Member

Choose a reason for hiding this comment

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

Can we remove this init() function, it's considered an anti-pattern?

Also, this modifies every http.DefaultTransport and I'm pretty sure we don't want to do that...!

Copy link
Member

Choose a reason for hiding this comment

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

Also, nobody is using defaultTransport locally.

Copy link
Member Author

Choose a reason for hiding this comment

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

defaultTransport is used for an argument with the function calls after. Unfortunately we need this to get some transport changes in for juju. juju tests currently require being able to change the DefaultTransport to add different schemes for test and replace the RoundTripper. Updating the tests needs to be done, but make take weeks of effort which we do not have at this time. Small updates can be make to use httptest instead and chip away at the problem. The follow on pr in juju/juju starts that.

Copy link
Member

@SimonRichardson SimonRichardson left a comment

Choose a reason for hiding this comment

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

Can we add a LP bug, this makes me sad.

@hmlanigan hmlanigan force-pushed the initial branch 2 times, most recently from dc40df1 to a436e6c Compare July 24, 2020 18:06
@hmlanigan
Copy link
Member Author

@SimonRichardson, agreed, makes me sad as well. https://bugs.launchpad.net/juju/+bug/1888888.

This code has been moved from juju/utils with a few differences:
there is one constructure which takes a config; does not cache proxy
settings; uses httptrace on http.Get methods.
@hmlanigan
Copy link
Member Author

!!build!!

@hmlanigan
Copy link
Member Author

Looking to creating a merge job and check merge job for this repo. Then will merge.

@hmlanigan
Copy link
Member Author

!!build!!

1 similar comment
@hmlanigan
Copy link
Member Author

!!build!!

@jujubot
Copy link
Contributor

jujubot commented Jul 28, 2020

Build finished.

@hmlanigan
Copy link
Member Author

$$merge$$

@jujubot
Copy link
Contributor

jujubot commented Jul 28, 2020

Build finished.

@hmlanigan
Copy link
Member Author

$$merge$$

@jujubot jujubot merged commit 3e6739b into juju:master Jul 28, 2020
@hmlanigan hmlanigan deleted the initial branch July 28, 2020 13:11
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.

3 participants