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

resolve: fix recursion #6087

Merged
merged 3 commits into from
Mar 19, 2019
Merged

resolve: fix recursion #6087

merged 3 commits into from
Mar 19, 2019

Conversation

Stebalien
Copy link
Member

  1. Recurse by default. This is what users expect and it should only un-break
    applications. We have a default limit of 32.
  2. When recursion is turned off, return the partial result. Otherwise, these
    commands are pretty much useless

fixes #5635
fixes #5585
fixes #4181
fixes #4293
fixes #6086

This is what users expect.

fixes #5635
fixes #5585
fixes #4181
fixes #4293
fixes #6086

License: MIT
Signed-off-by: Steven Allen <steven@stebalien.com>
Otherwise, non-recursive resolution is pretty much useless.

License: MIT
Signed-off-by: Steven Allen <steven@stebalien.com>
@Stebalien Stebalien requested a review from Kubuxu as a code owner March 16, 2019 02:28
@ghost ghost assigned Stebalien Mar 16, 2019
@ghost ghost added the status/in-progress In progress label Mar 16, 2019
@Stebalien
Copy link
Member Author

CC @Kubuxu and @lidel.

License: MIT
Signed-off-by: Steven Allen <steven@stebalien.com>
@Kubuxu
Copy link
Member

Kubuxu commented Mar 18, 2019

@Stebalien what do you think about adding to ResolvedPath information about the fact whether the resolution was final or not?

@Stebalien
Copy link
Member Author

@Kubuxu I'm not seeing how this relates to ResolvedPath. Currently, ResolvePath paths are always fully resolved.

@Kubuxu
Copy link
Member

Kubuxu commented Mar 19, 2019

What I mean: core/commands/name.ResolvedPath, IIRC before the commands rework it was possible to get partial path from the API (not via cli but via json).
But I aggree, unless we are able to return error and the partial path it would be problematic.

@Stebalien
Copy link
Member Author

But I aggree, unless we are able to return error and the partial path it would be problematic.

We could still return the partial path and then an error. However, unfortunately, javascript hard-codes whether or not the response to any given command is "streamed" or sent as one blob.

So, partial resolution would require a new flag.

@Kubuxu
Copy link
Member

Kubuxu commented Mar 19, 2019

Let's move with what we have.

@Stebalien
Copy link
Member Author

@Kubuxu So... can you approve this?

@Stebalien Stebalien merged commit 3f0bd78 into master Mar 19, 2019
@ghost ghost removed the status/in-progress In progress label Mar 19, 2019
@Stebalien
Copy link
Member Author

(well, I'll take that as approval)

@Stebalien Stebalien deleted the fix/recursive-resolve branch March 19, 2019 16:14
@Kubuxu
Copy link
Member

Kubuxu commented Mar 19, 2019

Sorry, I was reviewing codecov report and then forgot to click the approve.

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