I doubt it has any thing to do with the actual change. Any advice?

@doronp: I triggered a rebuild in CI. I will look into the test failures.

Current coverage is 54.96%

Merging #1097 into master will increase coverage by 0.06%

@@             master      #1097   diff @@
==========================================
  Files            77         77          
  Lines         12193      12188     -5   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits           6694       6699     +5   
+ Misses         4579       4569    -10   
  Partials        920        920          

Powered by Codecov. Last updated by eacedad...82446c9

@doronp Doesn't this break name lookup?

@aluzzardi Your actually right. Surprised the tests passed. We should add such a test. Now full Id, short ID and name should work in that order.

Thanks @doronp! Yeah there are no CLI tests - it's an example/debugging/advanced CLI, the typical way to use SwarmKit is through Docker (which has CLI tests).

I think we should try name before short ID: full ID -> name -> short ID.

The reason is a name is less ambiguous than a short ID: for instance if you have a service named "foo" and a short ID that starts with "foo", you are probably referred to the name rather than the short ID.

@aluzzardi I made name lookup come before Id prefix. Agree with your reasoning.

LGTM

ping @aluzzardi

LGTM

@aaronlehmann
Your last merge conflicted with this one because of one of its parents. I resolved that conflict.

Taking another look at this, I'm concerned that this introduces different behavior for networks relative to the other object types.

For example, getService looks like this:

func getService(ctx context.Context, c api.ControlClient, input string) (*api.Service, error) {
        // GetService to match via full ID.
        rg, err := c.GetService(ctx, &api.GetServiceRequest{ServiceID: input})
        if err != nil {
                // If any error (including NotFound), ListServices to match via ID prefix and name prefix.
                rl, err := c.ListServices(ctx,
                        &api.ListServicesRequest{
                                Filters: &api.ListServicesRequest_Filters{
                                        NamePrefixes: []string{input},
                                },
                        },
                )
                if err != nil {
                        return nil, err
                }

                if len(rl.Services) == 0 {
                        return nil, fmt.Errorf("service %s not found", input)
                }

                if l := len(rl.Services); l > 1 {
                        return nil, fmt.Errorf("service %s is ambiguous (%d matches found)", input, l)
                }

                return rl.Services[0], nil
        }
        return rg.Service, nil
}

It doesn't seem right for networks to support ID prefixes if services do not.

Let's update all object types to support ID prefix matches.

@aaronlehmann
1 - The naming 'NamePrefixes' is actually taken from your change :-) So I can revert it.
2 - I was planning on following with the services PR once this goes in. If you wish I can Do it sooner.

Thanks

Yes, I think we should do all object types at once.

@aaronlehmann Done. Please see #1194

Are we good to go with this and PR #1194?

@aaronlehmann are you happy with this? Can we make a decision on merge or close?

There should be a single PR that makes the relevant changes for all object types at once, including task, cluster, etc. We can't introduce inconsistencies between the object types.

Also, it would be very helpful to have more descriptive PR titles and summaries.

Alright, sounds like we can close this.

@doronp feel free to open a new PR with @aaronlehmann's suggestions and a better title and description.

@aaronlehmann @dperny for some background discussion on order of preference (and a CVE we once had in docker/docker), see the discussion here moby/moby#13286