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

ipfs config command with git-style option value getters and setters #16

Closed
wants to merge 1 commit into from

Conversation

dborzov
Copy link
Contributor

@dborzov dborzov commented Jul 30, 2014

Hi there! i wanted to add something small and simple and attempted to make the "ipfs config" command. The idea was to follow git's command line syntax style.

So now we can get specific config's key values with:

        $ipfs config datastore.path
        ~/.go-ipfs/datastore

And assign new values to the keys with:

        $ipfs config datastore.path ~/.go-ipfs/datastore

Plus, in the style similar to:

       git config --global --edit

we can open the config file in the default editor(as taken from $EDITOR environment variable):

       ipfs config edit

One thing to address in the future is that the config keys here are simply read (or assigned) from config's JSON, and just ignore the actual go-ipfs/config structures.
The other one is to add support for custom config file's path.

I would very much appreciate any feedback. Thanks!

Adds "ipfs config" command designed in git style.
See specific config's values with:
        $ipfs config datastore.path
        ~/.go-ipfs/datastore
Assign a new value with:
        $ipfs config datastore.path ~/.go-ipfs/datastore

Open the config file in your default editor(as taken from $EDITOR enviroment variable):
        ipfs config edit
@jbenet
Copy link
Member

jbenet commented Jul 30, 2014

This is great, and totally needed. Thanks! I'll CR shortly-- i'll probably have some comments.

One comment is that, right now, the main codebase is progressing in the dht branch. We can merge later, but might be annoying (thanks to my inconsistent go fmt).

Another comment is that i implemented something similar we can draw from in https://github.com/jbenet/data/blob/master/data_config.go months ago. It probably sucks (and uses yaml), but putting it here for reference.

@dborzov
Copy link
Contributor Author

dborzov commented Jul 30, 2014

Merging this PR into dht seems to be smooth enough. I pulled the dht branch and git cherry-picked the request's commit and it is all ff and fine.

@whyrusleeping
Copy link
Member

can you resubmit the pull request against the dht branch then? (not sure if github allows you to change the target branch on the fly)

var cfg interface{}
var exists bool

err = json.Unmarshal(buf, &cfg)
Copy link
Member

Choose a reason for hiding this comment

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

The more idiomatic go way of doing this would be to just open the file os.Open and then use json.NewDecoder to do the decoding.

fi,err := os.Open(filename)
if err != nil {//whatever}
dec := json.NewDecoder(fi)
var cfg map[string]interface{}
err := dec.Decode(&cfg)
if err != nil {//whatever}

(obviously written cleaner than my chicken scratch)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can rewrite it using json.Decoder, but I don't really see that much of a difference.
Could you please comment on why this way is preferable / more idiomatic?

Copy link
Member

Choose a reason for hiding this comment

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

@jmoiron does a good job explaining it in one of his articles: http://jmoiron.net/blog/crossing-streams-a-love-letter-to-ioreader/

Copy link
Member

Choose a reason for hiding this comment

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

That's a good post!

@dborzov I can rewrite it-- there's a few more stylistic things i'll fix that don't want to annoy with.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the link, that article makes good sense to me.

@jbenet
Copy link
Member

jbenet commented Jul 31, 2014

Awesome, merged manually in 4b1bae8...639d196 (onto master), thanks!

@jbenet jbenet closed this Jul 31, 2014
@jbenet
Copy link
Member

jbenet commented Jul 31, 2014

Note the new help message, for some of the changes I made.

> ipfs config
ipfs config [<key>] [<value>] - Get/Set ipfs config values.

    ipfs config <key>          - Get value of <key>
    ipfs config <key> <value>  - Set value of <key> to <value>
    ipfs config --show         - Show config file
    ipfs config --edit         - Edit config file in $EDITOR

Examples:

  Get the value of the 'datastore.path' key:

      ipfs config datastore.path

  Set the value of the 'datastore.path' key:

      ipfs config datastore.path ~/.go-ipfs/datastore

@dborzov
Copy link
Contributor Author

dborzov commented Jul 31, 2014

Great, the rewrite looks very nice, thanks!

There is a related thing I wanted to ask about.

There are hooks all over the place to pass the custom path to config file with a flag. There is also a TODO comment on that at cmd/ipfs/ipfs.go:69. Would it be easier to just read it off the ENV variable? Something like $IPFS_CONFIG? Then you can just achieve the same thing within the same line of shell command:

         IPFS_CONFIG=/my/path ipfs blah-blah

@jbenet
Copy link
Member

jbenet commented Aug 1, 2014

Hm, yeah, we could use an env variable. Am on the fence about that-- there are many programs on either camp. shrug I don't care much either way.

ribasushi pushed a commit that referenced this pull request Jul 4, 2021
Fix SIGSEGV due to incorrect usage of sync.Pool
ribasushi pushed a commit that referenced this pull request Jul 4, 2021
longfeiWan9 pushed a commit to longfeiWan9/go-ipfs that referenced this pull request Nov 18, 2021
Add "jpeg" as an alias to "jpg".
laurentsenta pushed a commit to laurentsenta/kubo that referenced this pull request Feb 25, 2022
laurentsenta pushed a commit to laurentsenta/kubo that referenced this pull request Feb 25, 2022
laurentsenta pushed a commit to laurentsenta/kubo that referenced this pull request Mar 4, 2022
laurentsenta pushed a commit to laurentsenta/kubo that referenced this pull request Mar 4, 2022
ariescodescream pushed a commit to ariescodescream/go-ipfs that referenced this pull request Apr 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants