Skip to content

Update service networks documentation#39495

Merged
cpuguy83 merged 1 commit intomoby:masterfrom
hannseman:network-attachment-config-docs
Nov 5, 2019
Merged

Update service networks documentation#39495
cpuguy83 merged 1 commit intomoby:masterfrom
hannseman:network-attachment-config-docs

Conversation

@hannseman
Copy link
Contributor

@hannseman hannseman commented Jul 11, 2019

This PR updates the documentation for the /services/ API. It fixes some issues around the Networks parameter.

The previous description stated that an array of names / ids could be passed when the API in reality expects objects in the form of NetworkAttachmentConfig. This is fixed by updating the description and adding a definition for NetworkAttachmentConfig.

Also it's quite confusing that Networks can both be specified in the service spec and in the TaskTemplate. From my understanding the correct way to pass Networks is through TaskTemplate:

func (c *Cluster) populateNetworkID(ctx context.Context, client swarmapi.ControlClient, s *types.ServiceSpec) error {
// Always prefer NetworkAttachmentConfigs from TaskTemplate
// but fallback to service spec for backward compatibility

I have not found any official information about Networks in the service spec being deprecated but it would probably be good to write something about this in the description for this parameter. Not really sure about the correct wording for this so I would glad to get some advice here.

The Networks parameter in the service spec also crashes when clicked, looks like this issue #32917. This makes it extra confusing with the current description about expecting an array of ids / names since the expected format can't be expanded.

The previous description stated that an array of names / ids could be passed when the API in reality expects objects in the form of NetworkAttachmentConfig. This is fixed by updating the description and adding a definition for NetworkAttachmentConfig.

Signed-off-by: Hannes Ljungberg <hannes@5monkeys.se>
@hannseman hannseman force-pushed the network-attachment-config-docs branch from d7cddec to 4d09fab Compare August 24, 2019 19:43
@codecov
Copy link

codecov bot commented Aug 24, 2019

Codecov Report

❗ No coverage uploaded for pull request base (master@2a64e34). Click here to learn what that means.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master   #39495   +/-   ##
=========================================
  Coverage          ?   37.32%           
=========================================
  Files             ?      609           
  Lines             ?    45224           
  Branches          ?        0           
=========================================
  Hits              ?    16878           
  Misses            ?    26057           
  Partials          ?     2289

@hannseman
Copy link
Contributor Author

Let me know if there's anything I can help with regarding this PR. Thanks!

@AkihiroSuda
Copy link
Member

cc @cpuguy83 @thaJeztah

Copy link
Member

@cpuguy83 cpuguy83 left a comment

Choose a reason for hiding this comment

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

LGTM

@cpuguy83
Copy link
Member

cpuguy83 commented Nov 5, 2019

re: Being able to set it two places, yes it is confusing. The method of setting it in the TaskTemplate I believe was added later for greater flexibility.

@hannseman
Copy link
Contributor Author

@cpuguy83 @AkihiroSuda thanks for the review and merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Docker API Error into documentation site

5 participants