Move 'fetch' option to 'image fetch'

[john-pettigrew]

The 'fetch' option belongs under 'image'. I left a hidden option 'rkt fetch' to make this a non-breaking change. Note: I had trouble getting all tests to run.

Fixes #2701

[ghost]

Can one of the admins verify this patch?

[lucab]

ok to test

[s-urbaniak]

@john-pettigrew thanks! Do you mind to reformat the commit messages according to https://github.com/coreos/rkt/blob/master/CONTRIBUTING.md#format-of-the-commit-message?

Also i think the change is small enough to fit in one commit.

[john-pettigrew]

Sure! I have now squashed the commits and fixed the commit message.

[tmrts]

@john-pettigrew please don't remove the fetch command completely, instead make it call the new function and print a deprecation message

[iaguis]

I'm uneasy about deprecating rkt fetch.

[tmrts]

@iaguis we're just moving it to image fetch where it makes more sense, what are your concerns?

[john-pettigrew]

@tmrts I did not remove it. I Just hid it from the command list. I can go ahead and add a deprecation message though.

[iaguis]

@iaguis we're just moving it to image fetch where it makes more sense, what are your concerns?

I know it makes more sense but it's a nice convenience, I think it should be an alias and there should be no deprecation warning.

[john-pettigrew]

@tmrts or are you saying that I should add a completely different function that is called on 'rkt fetch' that just calls the image fetch function?

[jonboulle]

We're not going to deprecate it for 1.x. Let's just alias it and we can
consider deprecation in a hypothetical 2.x.

On 6 July 2016 at 17:46, John Pettigrew notifications@github.com wrote:

@tmrts https://github.com/tmrts or are you saying that I should add a
completely different function that is called on 'rkt fetch' that just calls
the image fetch function?

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#2860 (comment), or mute
the thread
https://github.com/notifications/unsubscribe/ACewN8ShvQ6xxElZn7IXErRgPxxLxRXuks5qS83ugaJpZM4JCwG1
.

[tmrts]

@iaguis I can agree to it being just an alias

@john-pettigrew I missed the part where you aliased fetch to image fetch

[tmrts]

@iaguis LGTM if you don't have any objections

[yifan-gu]

-1 On this, IMO rkt fetch is convenient and causing no ambiguous, I am +1 on keeping it.
cc @robszumski

[robszumski]

-1 On this, IMO rkt fetch is convenient and causing no ambiguous, I am +1 on keeping it.

Agree that we should alias both. Basically add image fetch without affecting the UX of existing commands.

We're not going to deprecate it for 1.x. Let's just alias it and we can
consider deprecation in a hypothetical 2.x.

I do not think we should say anything about deprecation until we get closer to a 2.x and have this discussion.

[tmrts]

@robszumski so then the documentation should stay as is right?

[john-pettigrew]

Okay I can move the documentation back and hide the 'image fetch' option.

[tmrts]

Thank you John

On Thu, Jul 14, 2016 at 6:01 PM John Pettigrew notifications@github.com
wrote:

Okay I can move the documentation back and hide the 'image fetch' option.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#2860 (comment), or mute
the thread
https://github.com/notifications/unsubscribe/AE1S59V0al_3TjkuIXTjaSvfqynOjajrks5qVl0ogaJpZM4JCwG1
.

[robszumski]

Yeah, I think the docs are fine for now.

[john-pettigrew]

Done, let me know if I need to do anything else!

[alban]

The test failure in Jenkins is not related (#2432)

02:20:28 --- FAIL: TestSocketProxyd (7.49s)
02:20:28    rkt_socket_proxyd_test.go:200: read tcp [::1]:38508->[::1]:46705: read: connection reset by peer