Skip to content
This repository has been archived by the owner on Mar 9, 2022. It is now read-only.

add remote pinning services config #113

Merged
merged 2 commits into from
Dec 8, 2020
Merged

add remote pinning services config #113

merged 2 commits into from
Dec 8, 2020

Conversation

petar
Copy link
Contributor

@petar petar commented Sep 25, 2020

This is needed for ipfs/kubo#7661

Copy link
Member

@lidel lidel left a comment

Choose a reason for hiding this comment

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

This PR seems to duplicate already existing #104, so either we need to update it to follow this structure:

{ 
  Pinning: {
    RemoteServices: {
      "name" {
        ApiEndpoint: "https://pins.example.com/",
        ApiKey: "foobar", 
        Policies: ["mfs-root"]
      } 
    } 
  }
}

or rebase the #104 and close this one.

@petar
Copy link
Contributor Author

petar commented Nov 12, 2020

@lidel @aschmahmann This PR is what ipfs/kubo#7661 is using. The main difference here is that pinning services is a map[string]RemotePinService, as opposed to []RemotePinService. Any reason to prefer a slice?

@aschmahmann
Copy link
Contributor

I don't have a strong preference for slice vs map, I think what @lidel was asking for was to do Pinning.RemoteServices instead of just RemotePinServices

@lidel
Copy link
Member

lidel commented Nov 13, 2020

@petar I don't feel strongly either way, but we already use map[string] approach in Gateway.PublicGateways so if there is no good reason to go other way, I'd go with the same here.

Asks:

  • Rename structs to Pinning.RemoteServices
  • I'd go with somethign like ApiEndpoint and ApiKey in case we want to support Multiaddrs instead of URLs
  • No need to duplicate name, I believe it's enough to have it once, in key.

(updated my comment above to use it too)

@petar
Copy link
Contributor Author

petar commented Nov 17, 2020

@lidel @aschmahmann What is the consensus design for the Policy field? map[string]string? I am looking into it as I'll need it for the MFS pinning feature.

@lidel
Copy link
Member

lidel commented Nov 17, 2020

Initially we would have either none or only "all-in-mfs", but in the long run we want to be able to have more than one policy enabled (eg. "all-in-mfs" and "all-local-pins"),

so either []string or map[string]bool(?) would work for ipfs-webui (as long we can read them back via pin remote service ls --policy):

Policy: ["all-in-mfs", "all-local-pins"]

or

Policy: {
  mirrorMFS: true
  mirrorLocalPins: true
}

I don't know go(-ipfs) well enough to feel strongly either way – @aschmahmann thoughts?
Which will be easier to wire up and maintain?

@aschmahmann
Copy link
Contributor

aschmahmann commented Nov 17, 2020

I don't have particularly strong feelings here, but probably maps/structs (where they're both the same in JSON, but structs allows us to be stricter in our Go code). We can also use flag (see Swarm.Transports for an example) instead of boolean values to allow us to configure the defaults in go-ipfs (they'd default to false), although if we think the defaults will always be false I'm open to using a boolean here instead.

I tried to look at what our other config options do, such as how the gateways have

ipfs config --json Gateway.PublicGateways '{
    "ipfs.io": {
      "UseSubdomains": false,
      "Paths": ["/ipfs", "/ipns", "/api"]
    }
  }'

This of course is no help since we've used both options inside of a single command. Nonetheless, I think the "UseSubdomains" boolean is a little closer to what we're looking for here since structs like this are "typed" and so a little stricter and simpler to work with.

It could also make it easier to override defaults. Since maps ~ structs this means that if go-ipfs supports two policies "MirrorMFS" and "MirrorLocalPins" then we can say "by default local pins are duplicated remotely" (not necessarily a good idea, but for the sake of argument) and so MirrorLocalPins will default to true, while MirrorMFS defaults to false. If the user only sets MirrorMFS as true then MirrorLocalPins will remain the default instead of if it was an array where the user might not be sure if they were overriding MirrorLocalPins as false. Note I'm open to being told "having any remote pin default other than none is insane so just use an array to force users to override them all."

@lidel
Copy link
Member

lidel commented Nov 18, 2020

For what its worth, I can't come up with any policy that we would want to enable by default. IMO user agency takes a priority here. There should be no policies enabled: user needs to consciously opt-in.

@petar
Copy link
Contributor Author

petar commented Nov 23, 2020

@lidel @aschmahmann I took a first stab at the MFS policy flag. See what you think.

@lidel lidel force-pushed the petar/remotepin branch 2 times, most recently from ab9c56f to 280c4db Compare November 25, 2020 17:53
remotepin.go Outdated Show resolved Hide resolved
remotepin.go Outdated Show resolved Hide resolved
@aschmahmann
Copy link
Contributor

All MFS work has been moved to #116

Copy link
Member

@lidel lidel left a comment

Choose a reason for hiding this comment

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

LGTM, but we may want to simplify names (either now or when MFS PR lands)

remotepin.go Outdated Show resolved Hide resolved
Copy link
Contributor Author

@petar petar left a comment

Choose a reason for hiding this comment

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

LGTM

remotepin.go Outdated Show resolved Hide resolved
@aschmahmann aschmahmann merged commit 1a8f582 into master Dec 8, 2020
@aschmahmann aschmahmann deleted the petar/remotepin branch December 8, 2020 19:41
@aschmahmann aschmahmann mentioned this pull request Feb 18, 2021
73 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants