Skip to content

Do not add duplicate platform information to service spec#33867

Merged
cpuguy83 merged 1 commit intomoby:masterfrom
nishanttotla:fix-duplicate-platform-info
Jun 29, 2017
Merged

Do not add duplicate platform information to service spec#33867
cpuguy83 merged 1 commit intomoby:masterfrom
nishanttotla:fix-duplicate-platform-info

Conversation

@nishanttotla
Copy link
Copy Markdown
Contributor

Fix #33865

While updating services and querying the registry for platform information, updateServicePlatforms should not add platform information to the service when it already exists.

@nishanttotla
Copy link
Copy Markdown
Contributor Author

This must also be vendored into docker/cli.

@aaronlehmann
Copy link
Copy Markdown

I think we should overwrite the platform info when QueryRegistry is set, not try to merge with the previous values.

@nishanttotla
Copy link
Copy Markdown
Contributor Author

@aaronlehmann I agree, that makes sense. I added an overwrite option to updateServicePlatforms that is set to true for service updates. I don't see any reason to set it to false as of now (even though it is done for service create), but this might be useful to do in the future.

@aaronlehmann
Copy link
Copy Markdown

The overwrite parameter doesn't make sense to me. Either the platform info comes from the original spec or from the registry, but I don't see how the union of the two is the right result.

@thaJeztah
Copy link
Copy Markdown
Member

I don't see any reason to set it to false as of now (even though it is done for service create), but this might be useful to do in the future.

I'd prefer to rename the function to setServicePlatforms and don't make the overwrite optional. Adding features that "may be useful" at some point can easily lead to both dead code, and people not being sure if a feature can be removed or not.

@nishanttotla
Copy link
Copy Markdown
Contributor Author

Alright, I'll get rid of it. Y'all make sense.

@nishanttotla nishanttotla force-pushed the fix-duplicate-platform-info branch from 1ef6b01 to da85b62 Compare June 28, 2017 23:01
@aaronlehmann
Copy link
Copy Markdown

LGTM

Copy link
Copy Markdown
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

LGTM

we could add a simple unit test to prevent regressions, but I'm okay with having that as a follow up

@thaJeztah
Copy link
Copy Markdown
Member

hitting #33863

@cpuguy83 cpuguy83 merged commit fcaa79b into moby:master Jun 29, 2017
@nishanttotla nishanttotla deleted the fix-duplicate-platform-info branch June 29, 2017 18:32
thaJeztah added a commit to thaJeztah/cli that referenced this pull request Jul 2, 2017
Includes changes from;

- Fix handling of remote "git@" notation (moby/moby#33696)
- Move some `api` package functions away moby/moby#33798 (related to docker#236)
- Update go-connections dependency moby/moby#33814 (already vendored in docker#238)
- Set ping version even on error (moby/moby#33827)
- Do not add duplicate platform information to service spec (moby/moby#33867)
- Refactor MountPoint Setup function in volume.go (moby/moby#33890)

Signed-off-by: Sebastiaan van Stijn <[email protected]>
thaJeztah added a commit to thaJeztah/cli that referenced this pull request Jul 2, 2017
Includes changes from;

- Add a LastTagTime for images (moby/moby#31497)
- Fix handling of remote "git@" notation (moby/moby#33696)
- Move some `api` package functions away (moby/moby#33798) (related to docker#236)
- Set ping version even on error (moby/moby#33827)
- Do not add duplicate platform information to service spec (moby/moby#33867)
- Refactor MountPoint Setup function in volume.go (moby/moby#33890)

Signed-off-by: Sebastiaan van Stijn <[email protected]>
thaJeztah added a commit to thaJeztah/cli that referenced this pull request Jul 3, 2017
Includes changes from;

- Add a LastTagTime for images (moby/moby#31497)
- Fix handling of remote "git@" notation (moby/moby#33696)
- Move some `api` package functions away (moby/moby#33798) (related to docker#236)
- Set ping version even on error (moby/moby#33827)
- Do not add duplicate platform information to service spec (moby/moby#33867)
- Refactor MountPoint Setup function in volume.go (moby/moby#33890)

Signed-off-by: Sebastiaan van Stijn <[email protected]>
thaJeztah added a commit to thaJeztah/cli that referenced this pull request Jul 5, 2017
Includes changes from;

- Add a LastTagTime for images (moby/moby#31497)
- Fix handling of remote "git@" notation (moby/moby#33696)
- Move some `api` package functions away (moby/moby#33798) (related to docker#236)
- Set ping version even on error (moby/moby#33827)
- Do not add duplicate platform information to service spec (moby/moby#33867)
- Refactor MountPoint Setup function in volume.go (moby/moby#33890)

Signed-off-by: Sebastiaan van Stijn <[email protected]>
andrewhsu pushed a commit to docker-archive/docker-ce that referenced this pull request Jul 14, 2017
Includes changes from;

- Add a LastTagTime for images (moby/moby#31497)
- Fix handling of remote "git@" notation (moby/moby#33696)
- Move some `api` package functions away (moby/moby#33798) (related to docker/cli#236)
- Set ping version even on error (moby/moby#33827)
- Do not add duplicate platform information to service spec (moby/moby#33867)
- Refactor MountPoint Setup function in volume.go (moby/moby#33890)

Signed-off-by: Sebastiaan van Stijn <[email protected]>
Upstream-commit: 366d3ec971d8007c667e8d7dc8e35a346fb19539
Component: cli
@thaJeztah thaJeztah added this to the 17.06.0 milestone Jul 21, 2017
alshabib pushed a commit to alshabib/cli that referenced this pull request Aug 1, 2017
Includes changes from;

- Add a LastTagTime for images (moby/moby#31497)
- Fix handling of remote "git@" notation (moby/moby#33696)
- Move some `api` package functions away (moby/moby#33798) (related to docker#236)
- Set ping version even on error (moby/moby#33827)
- Do not add duplicate platform information to service spec (moby/moby#33867)
- Refactor MountPoint Setup function in volume.go (moby/moby#33890)

Signed-off-by: Sebastiaan van Stijn <[email protected]>
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.

5 participants