Skip to content
This repository has been archived by the owner on Aug 2, 2021. It is now read-only.

api, cmd: add pinning feature flag #1795

Merged
merged 3 commits into from
Sep 24, 2019
Merged

api, cmd: add pinning feature flag #1795

merged 3 commits into from
Sep 24, 2019

Conversation

acud
Copy link
Member

@acud acud commented Sep 23, 2019

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

@nonsense
Copy link
Contributor

LGTM, but there are no tests. Considering the timeframe we are under, I can understand that, but have you tried it locally?

@acud
Copy link
Member Author

acud commented Sep 23, 2019

LGTM, but there are no tests. Considering the timeframe we are under, I can understand that, but have you tried it locally?

of course.

Copy link
Member

@janos janos left a 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.

@@ -106,7 +106,6 @@ func NewServer(api *api.API, pinAPI *pin.API, corsString string) *Server {
server := &Server{api: api, pinAPI: pinAPI}

defaultMiddlewares := []Adapter{
RecoverPanic,
Copy link
Member

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?

Copy link
Member Author

@acud acud Sep 24, 2019

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?

api/http/middleware.go Show resolved Hide resolved
Copy link
Member

@janos janos left a comment

Choose a reason for hiding this comment

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

LGTM

@acud acud merged commit 79a7e6b into master Sep 24, 2019
@acud acud deleted the pinning-ff branch September 24, 2019 11:42
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use a feature flag for pinning and disable it by default
3 participants