-
Notifications
You must be signed in to change notification settings - Fork 14
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
refactor: unify http client creation #448
Conversation
Signed-off-by: Suraj Shirvankar <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe i miss something....
why not just daemon.downloadAndValidateImage()
call to utils.doTLSRequest()
?
the method post vs get can be an argument
happy to discuss ....
@glimchb I kept them seperate because there were some extra http headers (basic auth etc) being passed in the post request, no sure if that was needed in the get version. |
sztp-agent/pkg/secureagent/agent.go
Outdated
@@ -171,3 +184,25 @@ func (a *Agent) SetContentTypeReq(ct string) { | |||
func (a *Agent) SetProgressJSON(p ProgressJSON) { | |||
a.ProgressJSON = p | |||
} | |||
|
|||
func NewHttpClient(bootstrapTrustAnchorCert string, deviceEndEntityCert string, devicePrivateKey string) http.Client { | |||
caCert, _ := os.ReadFile(bootstrapTrustAnchorCert) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's move this from agent
to tls
?
so this should be separate PR or separate commit just for injection |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so NewHttpClient()
should move to another file - right ?
b8e5e6e
to
0b7a567
Compare
Signed-off-by: Suraj Shirvankar <[email protected]>
0b7a567
to
48825de
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
@h0lyalg0rithm I wanna merge this
please rebase ? |
LGTM |
Proposed changes
Unifies the httpclient used across the application.
No validation of server certificate
Types of changes
What types of changes does your code introduce to the repo? Put an
x
in the boxes that applyChecklist
Put an
x
in the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your code.