Skip to content
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: create config automatically if not provided #151

Merged
merged 14 commits into from
Oct 24, 2024

Conversation

SgtPooki
Copy link
Member

@SgtPooki SgtPooki commented Sep 23, 2024

Title

feat: create config automatically if not provided

Description

  • splits and organizes some of the code in src/index.ts
  • adds new configuration creation step if --config is not provided when starting up.
    • creates new convention for ~/.config/amino directory for storing configuration files

Notes & open questions

  • Any better suggestions for config path or file name?
    • updated to ~/.config/amino as the root. may update further?
  • Do we want to change defaults in example-config.json?
  • Do we want to use this chance to update config file to use more direct js-libp2p config name instead of the Kubo convention?

Change checklist

  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation if necessary (this includes comments as well)
  • I have added tests that prove my fix is effective or that my feature works

@SgtPooki SgtPooki linked an issue Sep 23, 2024 that may be closed by this pull request
Copy link
Member Author

@SgtPooki SgtPooki left a comment

Choose a reason for hiding this comment

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

self review

@achingbrain
Copy link
Member

https://stackoverflow.com/questions/47819464/where-to-save-configuration-data-files-on-gnu-linux suggests storing config under ~/.config because it's what XDG has specified and seems popular?

.js-libp2p as a directory name seems like a bit of an overreach, perhaps ~/.config/amino/config.json?

@SgtPooki
Copy link
Member Author

stackoverflow.com/questions/47819464/where-to-save-configuration-data-files-on-gnu-linux suggests storing config under ~/.config because it's what XDG has specified and seems popular?

Will update

.js-libp2p as a directory name seems like a bit of an overreach, perhaps ~/.config/amino/config.json?

Maybe ~/.config/amino/js-config.json in order to not squat on the namespace? but i'll push an update for this

@SgtPooki
Copy link
Member Author

Maybe ~/.config/amino/js-config.json in order to not squat on the namespace? but i'll push an update for this

~/.config/amino/bootstrapper.json would probably be better?

@achingbrain
Copy link
Member

Also, let's use jsonc-parser or some other jsonc parser so we can have comments in our config files..

Maybe ~/.config/amino/js-config.json in order to not squat on the namespace?

We're already squatting on the binary name, so just ~/.config/amino/config.json didn't seem like a terrible location 😉

@SgtPooki
Copy link
Member Author

FYI @achingbrain we do have ~/.config/@libp2p from your protocol-adventure tool. do we want to re-use that?

image

@achingbrain
Copy link
Member

Oh yeah, maybe we'd be better internet citizens if we did that.

If we use the module name then ~/.config/@libp2p/amino-dht-bootstrapper/config.json would be the one I suppose?

Copy link
Member Author

@SgtPooki SgtPooki left a comment

Choose a reason for hiding this comment

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

self review

src/utils/auto-config.ts Outdated Show resolved Hide resolved
@2color
Copy link
Contributor

2color commented Sep 23, 2024

Do we want to use this chance to update config file to use more direct js-libp2p config name instead of the Kubo convention?

I think this is a good idea. I imagine that most users of this are coming from js-libp2p, for which it would more intuitive to follow js-libp2p config naming.

Copy link
Member Author

@SgtPooki SgtPooki left a comment

Choose a reason for hiding this comment

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

self review

src/utils/load-config.ts Outdated Show resolved Hide resolved
src/utils/auto-config.ts Outdated Show resolved Hide resolved
src/utils/auto-config.ts Outdated Show resolved Hide resolved
@SgtPooki
Copy link
Member Author

SgtPooki commented Oct 9, 2024

@achingbrain @2color this is ready for review again: merged from main and updated config to use js-libp2p config keys. One non-normal item in the config is the identity property

@SgtPooki SgtPooki requested a review from 2color October 9, 2024 17:19
src/utils/default-config.ts Outdated Show resolved Hide resolved
},
bootstrap: {
list: [
'/dns4/am6.bootstrap.libp2p.io/tcp/443/wss/p2p/QmbLHAnMoJPWSCR5Zhtx6BHJX9KiKNN6tpvbUcqanj75Nb',
Copy link
Contributor

@2color 2color Oct 11, 2024

Choose a reason for hiding this comment

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

I would add the full list of defaults also used by Helia:

    '/dnsaddr/bootstrap.libp2p.io/p2p/QmNnooDu7bfjPFoTZYxMNLWUQJyrVwtbZg5gBMjTezGAJN',
    '/dnsaddr/bootstrap.libp2p.io/p2p/QmbLHAnMoJPWSCR5Zhtx6BHJX9KiKNN6tpvbUcqanj75Nb',
    '/dnsaddr/bootstrap.libp2p.io/p2p/QmcZf59bWwK5XFi76CZX8cbJ4BhTzzA3gU1ZjYZcYW3dwt',

    // va1 is not in the TXT records for _dnsaddr.bootstrap.libp2p.io yet
    // so use the host name directly
    '/dnsaddr/va1.bootstrap.libp2p.io/p2p/12D3KooWKnDdG3iXw9eTFijk3EWSunZcFi54Zka4wmtqtt6rPxc8',
    '/ip4/104.131.131.82/tcp/4001/p2p/QmaCpDMGvV2BGHeYERUEnRQAwe3N8SzbUtfsmvsqQLuvuJ'

https://github.com/ipfs/helia/pull/649/files#diff-1b64c902120625f3cd192eb7b8f10b323c4952735118b52ed87098ab4f7a6526R4-R10

Copy link
Member Author

Choose a reason for hiding this comment

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

addressed in #201

src/utils/auto-config.ts Outdated Show resolved Hide resolved
src/utils/auto-config.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@2color 2color left a comment

Choose a reason for hiding this comment

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

LGTM

@2color
Copy link
Contributor

2color commented Oct 11, 2024

I took the liberty of pushing some small fixes.

Overall, looks great and makes it much easier to run without much prior knowledge,

@SgtPooki SgtPooki merged commit f869934 into main Oct 24, 2024
20 checks passed
@SgtPooki SgtPooki deleted the 146-feat-auto-config-for-people-starting-using-npx branch October 24, 2024 16:09
github-actions bot pushed a commit that referenced this pull request Oct 24, 2024
## [1.7.0](v1.6.3...v1.7.0) (2024-10-24)

### Features

* create config automatically if not provided ([#151](#151)) ([f869934](f869934))

### Trivial Changes

* update project ([1a49861](1a49861))
Copy link

🎉 This PR is included in version 1.7.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feat: auto-config for people starting using npx
3 participants