Skip to content
This repository has been archived by the owner on Jun 20, 2024. It is now read-only.

[proxy] Handle null HostConfigs gracefully in start #1488

Merged
merged 1 commit into from
Oct 1, 2015
Merged

[proxy] Handle null HostConfigs gracefully in start #1488

merged 1 commit into from
Oct 1, 2015

Conversation

2opremio
Copy link
Contributor

Fixes #1481

@2opremio
Copy link
Contributor Author

@paulbellamy Please review

@paulbellamy
Copy link
Contributor

Could you just remove the & on the unmarshal call? would that be simpler?

I'd pick whichever's closer to how we handle unmarshal in the other interceptors.

@paulbellamy
Copy link
Contributor

Also, what's the -o /dev/null -w "%{http_code}\n" to curl?

@paulbellamy
Copy link
Contributor

Also, does curl fail on response codes >= 400?

Edit: Ah, curl can be made to with --fail, so maybe we should use that and do assert_raises "docker_api_on ..."

rade
rade reviewed Sep 30, 2015
View reviewed changes
test/config.sh
@@ -106,7 +106,7 @@ docker_api_on() {
data=$4
shift 4
[ -z "$DEBUG" ] || greyly echo "Docker (API) on $host:$DOCKER_PORT: $method $url" >&2
echo -n "$data" | curl -s -X "$method" -H Content-Type:application/json "http://$host:$DOCKER_PORT/v1.15$url" -d @-
echo -n "$data" | curl -s -o /dev/null -w "%{http_code}\n" -X "$method" -H Content-Type:application/json "http://$host:$DOCKER_PORT/v1.15$url" -d @-

This comment was marked as abuse.

Sign in to view

This comment was marked as abuse.

Sign in to view

This comment was marked as abuse.

Sign in to view

This comment was marked as abuse.

Sign in to view

This comment was marked as abuse.

Sign in to view

@2opremio
Copy link
Contributor Author

Could you just remove the & on the unmarshal call? would that be simpler?

Yep, that would be simpler, good point.

Also, what's the -o /dev/null -w "%{http_code}\n" to curl?

It prints the status code of the curl request

@2opremio 2opremio force-pushed the 1481-proxy-null-hostconfig branch from a36779b to 79a8c6a Compare September 30, 2015 19:11
@2opremio
Copy link
Contributor Author

@paulbellamy @rade PTAL

paulbellamy
paulbellamy reviewed Oct 1, 2015
View reviewed changes
proxy/start_container_interceptor.go
@@ -35,7 +35,7 @@ func (i *startContainerInterceptor) InterceptRequest(r *http.Request) error {
}
}

if err := marshalRequestBody(r, hostConfig); err != nil {
if err := marshalRequestBody(r, *hostConfig); err != nil {

This comment was marked as abuse.

Sign in to view

This comment was marked as abuse.

Sign in to view

This comment was marked as abuse.

Sign in to view

This comment was marked as abuse.

Sign in to view

@2opremio 2opremio assigned paulbellamy and unassigned 2opremio Oct 1, 2015
@rade rade force-pushed the 1481-proxy-null-hostconfig branch from 79a8c6a to c24dc66 Compare October 1, 2015 10:41
@rade rade assigned rade and unassigned paulbellamy Oct 1, 2015
rade added a commit that referenced this pull request Oct 1, 2015
@rade
Merge pull request #1488 from weaveworks/1481-proxy-null-hostconfig
a3484cc 
[proxy] Handle null HostConfigs gracefully in start

Fixes #1481.
@rade rade merged commit a3484cc into master Oct 1, 2015
@paulbellamy paulbellamy deleted the 1481-proxy-null-hostconfig branch October 1, 2015 11:09
@rade rade added this to the 1.2.0 milestone Oct 1, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees

@rade rade

Labels
None yet
Projects
None yet
Milestone
1.2.0
Development

Successfully merging this pull request may close these issues.
3 participants
@2opremio @paulbellamy @rade