Skip to content

Move some api package functions away#33798

Merged
vdemeester merged 1 commit intomoby:masterfrom
vdemeester:move-some-api-code-away
Jun 24, 2017
Merged

Move some api package functions away#33798
vdemeester merged 1 commit intomoby:masterfrom
vdemeester:move-some-api-code-away

Conversation

@vdemeester
Copy link
Copy Markdown
Member

@vdemeester vdemeester commented Jun 23, 2017

  • DisplayablePorts is a cli function, moving to docker/cli
  • Move MatchesContentType to the only package using it,
    api/server/httputils (and remove the deps on logrus for api package)

Related PR in docker/cli : docker/cli#236

Signed-off-by: Vincent Demeester [email protected]

@vdemeester vdemeester mentioned this pull request Jun 23, 2017
Merged
thaJeztah
thaJeztah approved these changes Jun 23, 2017
View reviewed changes
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.

one nit, but LGTM

Comment thread api/server/httputils/httputils.go Outdated
Copy link
Copy Markdown
Member

@thaJeztah thaJeztah Jun 23, 2017

Choose a reason for hiding this comment

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

should this go to the top?

Copy link
Copy Markdown
Member Author

@vdemeester vdemeester Jun 23, 2017

Choose a reason for hiding this comment

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

ahah yes it should 😛

dnephin
dnephin approved these changes Jun 23, 2017
View reviewed changes
Copy link
Copy Markdown
Member

@dnephin dnephin left a comment

Choose a reason for hiding this comment

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

LGTM with moved import

cpuguy83
cpuguy83 reviewed Jun 23, 2017
View reviewed changes
Comment thread api/server/httputils/httputils.go Outdated
Copy link
Copy Markdown
Member

@cpuguy83 cpuguy83 Jun 23, 2017

Choose a reason for hiding this comment

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

Does this need to be exported?

Copy link
Copy Markdown
Member Author

@vdemeester vdemeester Jun 23, 2017

Choose a reason for hiding this comment

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

Probably not 😇

@vdemeester vdemeester force-pushed the move-some-api-code-away branch from 7b40ccf to 0fcc78e Compare June 23, 2017 17:36
@vdemeester
Move some api package functions away
565aa41 
- DisplayablePorts is a `cli` function, moving to `docker/cli`
- Move MatchesContentType to the only package using it,
  `api/server/httputils` (and remove the deps on logrus for `api` package)

Signed-off-by: Vincent Demeester <[email protected]>
@vdemeester vdemeester force-pushed the move-some-api-code-away branch from 0fcc78e to 565aa41 Compare June 23, 2017 17:37
@vdemeester
Copy link
Copy Markdown
Member Author

vdemeester commented Jun 23, 2017

@cpuguy83 @dnephin updated 👼

cpuguy83
cpuguy83 approved these changes Jun 23, 2017
View reviewed changes
Copy link
Copy Markdown
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

mlaventure
mlaventure approved these changes Jun 23, 2017
View reviewed changes
Copy link
Copy Markdown
Contributor

@mlaventure mlaventure left a comment

Choose a reason for hiding this comment

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

LGTM

@thaJeztah
Copy link
Copy Markdown
Member

thaJeztah commented Jun 24, 2017

docker-py looks to fail on experimental and powerpc;

01:41:31 =================================== FAILURES ===================================
01:41:31 _________________ ServiceTest.test_create_service_with_secret __________________
01:41:31 /docker-py/tests/integration/api_service_test.py:429: in test_create_service_with_secret
01:41:31     container = self.get_service_container(name)
01:41:31 /docker-py/tests/integration/api_service_test.py:41: in get_service_container
01:41:31     all=include_stopped
01:41:31 /docker-py/docker/api/container.py:189: in containers
01:41:31     res = self._result(self._get(u, params=params), True)
01:41:31 /docker-py/docker/utils/decorators.py:46: in inner
01:41:31     return f(self, *args, **kwargs)
01:41:31 /docker-py/docker/api/client.py:183: in _get
01:41:31     return self.get(url, **self._set_request_timeout(kwargs))
01:41:31 /usr/lib/python2.7/dist-packages/requests/sessions.py:469: in get
01:41:31     return self.request('GET', url, **kwargs)
01:41:31 /usr/lib/python2.7/dist-packages/requests/sessions.py:457: in request
01:41:31     resp = self.send(prep, **send_kwargs)
01:41:31 /usr/lib/python2.7/dist-packages/requests/sessions.py:569: in send
01:41:31     r = adapter.send(request, **kwargs)
01:41:31 /usr/lib/python2.7/dist-packages/requests/adapters.py:407: in send
01:41:31     raise ConnectionError(err, request=request)
01:41:31 E   ConnectionError: ('Connection aborted.', BadStatusLine("''",))
01:41:31 ========= 1 failed, 244 passed, 8 skipped, 3 xfailed in 394.97 seconds =========
01:41:31 ---> Making bundle: .integration-daemon-stop (in bundles/17.06.0-dev/test-docker-py)
01:41:31 +++++ cat bundles/17.06.0-dev/test-docker-py/docker.pid
01:41:31 ++++ kill 14936

not sure if it's related, but both fail on the same test, so could be

@thaJeztah thaJeztah added status/2-code-review status/failing-ci Indicates that the PR in its current state fails the test suite and removed status/4-merge labels Jun 24, 2017
@vdemeester vdemeester mentioned this pull request Jun 24, 2017
Open
@vdemeester vdemeester removed rebuild/experimental status/failing-ci Indicates that the PR in its current state fails the test suite labels Jun 24, 2017
@vdemeester
Copy link
Copy Markdown
Member Author

vdemeester commented Jun 24, 2017

@thaJeztah Restarted the build and it's now green.. Seems unrelated 😓

@vdemeester vdemeester merged commit d311a3a into moby:master Jun 24, 2017
@vdemeester vdemeester deleted the move-some-api-code-away branch June 24, 2017 12:30
thaJeztah added a commit to thaJeztah/cli that referenced this pull request Jul 2, 2017
@thaJeztah
Bump moby to 87df0e533b619c088091fd1e2310e92bb9a24822
e89791c 
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
@thaJeztah
Bump moby to 87df0e533b619c088091fd1e2310e92bb9a24822
f7b5892 
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
@thaJeztah
Bump moby to 87df0e533b619c088091fd1e2310e92bb9a24822
1586738 
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 thaJeztah mentioned this pull request Jul 3, 2017
Merged
thaJeztah added a commit to thaJeztah/cli that referenced this pull request Jul 5, 2017
@thaJeztah
Bump moby to 87df0e533b619c088091fd1e2310e92bb9a24822
366d3ec 
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
@thaJeztah
Bump moby to 87df0e5
542cf42 
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
alshabib pushed a commit to alshabib/cli that referenced this pull request Aug 1, 2017
@thaJeztah @alshabib
Bump moby to 87df0e533b619c088091fd1e2310e92bb9a24822
710e182 
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

Reviewers

@cpuguy83 cpuguy83 cpuguy83 approved these changes

@thaJeztah thaJeztah thaJeztah approved these changes

+2 more reviewers

@dnephin dnephin dnephin approved these changes

@mlaventure mlaventure mlaventure approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

status/4-merge

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@vdemeester @thaJeztah @dnephin @cpuguy83 @mlaventure @GordonTheTurtle