printExport support for fish-shell #295
Conversation
|
I think the discussion on boot2docker/boot2docker#585 is relevant here. I'm very much +1 on the feature, but I think it could be cleaner if we could get rid of that outer |
|
@tianon awesome! I'll update the pull request soon with the refactoring. Thanks! |
|
Awesome. This would be really useful for fish shell users :). 👍 |
|
@douglascamata I'm prepping to do a 1.3.1 and I'd really like to see this in it 😄 Do you mind if I just steal your commit and take over from here? How much of the refactor did you get a chance to get into? |
|
@tianon I forgot to do it (sorry), gimme just a few minutes. |
|
@douglascamata sure! 😄 |
|
@tianon I'll use bash syntax as |
|
Absolutely, that sounds reasonable to me. |
|
(maybe even just a comment out to the side of |
|
Would you mind duplicating the outer |
|
(ie, |
|
@tianon okay 👍 |
|
@tianon sorry for the broken builds, hahahaha. |
|
No worries! LGTM @gmlewis @Moghedrin care to review this one too? 😄 |
|
(and thanks so much for the quick turnaround on this @douglascamata 😄) |
|
@tianon it's a pleasure to contribute, even more if it's with something you use daily and the maintainers are active and cool 👍 |
| } else { | ||
| fmt.Printf(" export %s=%s\n", name, value) | ||
| fmt.Printf(" set -x %s\n", name) |
There was a problem hiding this comment.
Don't you want to use the value here somehow?
There was a problem hiding this comment.
@gmlewis can't believe I let this... fixed!
There was a problem hiding this comment.
hahahaha, how embarrassing on my part for missing that - this is why we like to double up on reviews ❤️
| } else { | ||
| fmt.Printf(" export %s=%s\n", name, value) | ||
| fmt.Printf(" set -x %s %s\n", name, value) |
There was a problem hiding this comment.
Shouldn't there still be an = here? (ie, set -x %s=%s)
There was a problem hiding this comment.
(don't know Fish scripting at all :D)
There was a problem hiding this comment.
@tianon no, sir. Fish syntax for set is like: set -x DOCKER_HOST tcp://192.168.59.103:2376
There was a problem hiding this comment.
Cool, thanks for clarifying. 👍
|
re-LGTM |
|
@gmlewis wanna give it another once-over? 👼 |
| @@ -193,12 +194,19 @@ func checkEnvironment(socket, certPath string) bool { | |||
|
|
|||
| func printExport(socket, certPath string) { | |||
| for name, value := range exports(socket, certPath) { | |||
| if os.Getenv(name) != value { | |||
There was a problem hiding this comment.
One thing we seem to have lost here is the automatic silencing.
In other words, if a variable is already set correctly, no message was output.
Do we want to preserve that functionality?
There was a problem hiding this comment.
Yeah, this was removed on purpose. 😄
See the discussion at boot2docker/boot2docker#585, especially:
there's no harm in exporting the same value again :)
(although the explicit "unset"s are nice, so keeping those and expanding
them to include all possible vars as either an "export XYZ=..." or an
explicit "unset XYZ" would be superb)
There was a problem hiding this comment.
Ah, sorry I missed that. OK, great.
LGTM
|
Thanks @gmlewis ❤️ (and no worries - can't expect you to watch everything everywhere 😄) |
printExport support for fish-shell
|
Thanks, @tianon ! Yeah, you've probably noticed that I follow the code more than I follow the conversations. :-) |
|
Yeah, and that's perfect - we appreciate your presence very much! :D |
…ault docker build --rm is now default
|
so how do i eval it in fish? |
|
I use this command for now: boot2docker shellinit | sed s/"="/" "/ | awk '{print "set -gx " $2 " \"" $3 "\""}' | sourcealso you can make a function to do that automatically: function boot2docker_shellinit
boot2docker shellinit | sed s/"="/" "/ | awk '{print "set -gx " $2 " \"" $3 "\""}' | source;
end |
This is, basically, just an upstream update of #260. I just didn't used @kevinbin commits 'cause I couldn't find his repo anymore to merge with my remote.