-
Notifications
You must be signed in to change notification settings - Fork 110
Conversation
LGTM, but there are no tests. Considering the timeframe we are under, I can understand that, but have you tried it locally? |
of course. |
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.
It would be good to update cmd/swarm/config_test.go tests to at least check if the flag is functional.
api/http/server.go
Outdated
@@ -106,7 +106,6 @@ func NewServer(api *api.API, pinAPI *pin.API, corsString string) *Server { | |||
server := &Server{api: api, pinAPI: pinAPI} | |||
|
|||
defaultMiddlewares := []Adapter{ | |||
RecoverPanic, |
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.
We do not need RecoverPanic adapter?
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.
I'm inclined to remove it at this point. I think we should not catch panics as it might eclipse bugs and create possible database inconsistencies.
WDYT @janos?
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
This PR adds a feature flag to enable/disable pinning on a node.
Nodes would now be started with pinning disabled on default in order to simplify deployment configuration for those running gateways (in order to prevent external users pinning content through gateways, therefore possibly overloading gateway storage and bandwidth).
Users that would like to use this feature should bring their nodes up with
--enable-pinning
flag.fixes #1732