Skip to content
This repository has been archived by the owner on Feb 12, 2024. It is now read-only.

Add config profile endpoint and CLI #2165

Merged
merged 21 commits into from
Oct 6, 2019
Merged

Add config profile endpoint and CLI #2165

merged 21 commits into from
Oct 6, 2019

Conversation

dirkmc
Copy link
Contributor

@dirkmc dirkmc commented Jun 6, 2019

@dirkmc dirkmc changed the title Add config profile endpoint and CLI WIP: Add config profile endpoint and CLI Jun 6, 2019
@dirkmc dirkmc changed the title WIP: Add config profile endpoint and CLI Add config profile endpoint and CLI Jun 6, 2019
@dirkmc dirkmc requested a review from alanshaw June 6, 2019 20:04
@dirkmc
Copy link
Contributor Author

dirkmc commented Jun 7, 2019

The go implementation stores a backup of the config every time a profile is applied. I'm not sure that's necessary, @alanshaw do you think we need it?

@alanshaw
Copy link
Member

alanshaw commented Sep 4, 2019

FYI @dirkmc I have rebased and updated deps

src/core/components/config.js Outdated Show resolved Hide resolved
src/core/components/config.js Outdated Show resolved Hide resolved
src/core/components/config.js Outdated Show resolved Hide resolved
src/core/components/config.js Outdated Show resolved Hide resolved
src/core/components/config.js Outdated Show resolved Hide resolved
const out = await ipfs('config profile apply server')
expect(out).not.includes('PrivKey')
})
})
Copy link
Member

Choose a reason for hiding this comment

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

Can these just be interface-ipfs-core tests?

Copy link
Member

Choose a reason for hiding this comment

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

I'm hesitant for two reasons:

  1. We test to see if the profile was correctly applied by inspecting the config which requires the tests to have knowledge of what the profiles are - as implemented this would involve adding ipfs as a dependency of the interface tests which it is not currently
  2. There are CLI features that aren't in the interface definition (for better or for worse) like listing available profiles

Copy link
Member

Choose a reason for hiding this comment

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

I've exposed profile definitions over the http interface so these can be moved now, hooray!

@alanshaw
Copy link
Member

alanshaw commented Sep 5, 2019

@hacdias @olizilla @lidel we're adding profiles to js-ipfs. I'd like to add a desktop profile - what would that look like? Is desktop the default? What changes to the browser config should we make? Should we also have a browser profile so you can switch to it in other environments? Are we missing any other profiles? Please help 🙏

@lidel
Copy link
Member

lidel commented Sep 5, 2019

Prior art in go-ipfs/docs/config.md#profiles:
There is no desktop profile in go-ipfs (yet?), for now we manually apply config changes via ipfs-desktop:

I think we could scope initial js-ipfs profiles to:

  • server - similar to one in go-ipfs, disables preload, disables default signaling server, no dialing to local networks, no gc and mdns is also off (not sure about relay, perhaps relay should be a separate profile)
  • desktop - settings applied by ipfs-desktop (mdns, dht client, gc on), in future could be also a relay client mode
  • browser - enable preload, enable signaling via -star (in future relay), enable gc
  • test - similar to one in go-ipfs: disables things like bootstrap, preload, mdns to speed up tests etc (would remove a lot of boilerplate in tests going forward)

Hope this helps as a starting point :)

@achingbrain achingbrain mentioned this pull request Sep 25, 2019
52 tasks
@achingbrain
Copy link
Member

Merged master into this PR to fix the merge conflicts. Still needs a bunch of work to address the PR comments and probably to integrate with #2428

@achingbrain
Copy link
Member

I wonder if we could have an offline profile that didn't start a libp2p node at all? That would probably speed up the tests pretty dramatically as only a few of them actually require networking.

@achingbrain
Copy link
Member

Anyone know why the method this adds is ipfs.config.profile and not ipfs.config.applyProfile?

To me at least, it's a bit more obvious what the second one is going to do.

@achingbrain
Copy link
Member

achingbrain commented Oct 2, 2019

Or better yet:

Promise<Array<{ name: String, description: String }>> ipfs.config.profiles.list()
Promise<void> ipfs.config.profiles.apply(name, { dryRun: Boolean })

@achingbrain
Copy link
Member

achingbrain commented Oct 3, 2019

The functions in my previous comment are implemented so the list of available profiles is now exposed over the http api.

Depends on:

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.

RFC: js-ipfs profiles
4 participants