-
Notifications
You must be signed in to change notification settings - Fork 209
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
feat: Upgrade to latest ffresty with mTLS #1290
Conversation
Signed-off-by: Enrique Lacal <enrique.lacal@kaleido.io>
Signed-off-by: Enrique Lacal <enrique.lacal@kaleido.io>
0f59d93
to
82ae1e2
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.
Changes look great @EnriqueL8
If you wouldn't mind including the corresponding tests for coverage as well before we merge.
Not sure why the code coverage bot didn't run on your branch to help with that 🤔
Toggling code coverage in your vscode for each of the affected packages should help though.
client, err := ffresty.New(ctx, localConfig) | ||
|
||
if err != nil { | ||
return nil, err |
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.
Looks like an extra test is needed to retain coverage and cover this branch.
Signed-off-by: Enrique Lacal <enrique.lacal@kaleido.io>
Signed-off-by: Enrique Lacal <enrique.lacal@kaleido.io>
@peterbroadhurst let's hold off on this one until I get the wsclient changes in |
Signed-off-by: Enrique Lacal <enrique.lacal@kaleido.io>
Signed-off-by: Enrique Lacal <enrique.lacal@kaleido.io>
@@ -74,6 +74,8 @@ const ( | |||
func (e *Ethereum) InitConfig(config config.Section) { | |||
e.ethconnectConf = config.SubSection(EthconnectConfigKey) | |||
wsclient.InitConfig(e.ethconnectConf) | |||
ffresty.InitConfig(e.ethconnectConf) |
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.
Why is this now needed @EnriqueL8?
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 we hide that underneath the init config from the wsclient since wsClient.InitConfig
calls ffresty.InitConfig
. So if we ever want to separate the configs for the ws and ffresty we would need to call it here, but since they are the same today I can remove this line
Signed-off-by: Enrique Lacal <enrique.lacal@kaleido.io>
Codecov Report
@@ Coverage Diff @@
## main #1290 +/- ##
===========================================
- Coverage 100.00% 99.94% -0.06%
===========================================
Files 308 308
Lines 20692 20721 +29
===========================================
+ Hits 20692 20709 +17
- Misses 0 6 +6
- Partials 0 6 +6
|
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.
Thanks @EnriqueL8
Depends on:
Once those are in I'll update this one