Added feature for placement key in v3
#813
Conversation
|
@surajnarwade, thank you for the pull request! We'll request some people to review your PR. @cdrage and @kadel, please review this. |
k8s-ci-robot
added
cncf-cla: yes
|
@surajnarwade Code looks good, but can you add some integration / cmd tests? Then LGTM from me 👍 |
surajnarwade
force-pushed
the
placementv3
branch
from
9324bd9
to
06e8b87
Compare
k8s-ci-robot
added
size/L
|
@cdrage , tests added :) |
surajnarwade
force-pushed
the
placementv3
branch
from
06e8b87
to
0b13c47
Compare
|
One last thing, can you update Otherwise, code LGTM. Once docs are updated let's merge. |
surajnarwade
force-pushed
the
placementv3
branch
2 times, most recently
from
2f5c036
to
879e6f4
Compare
|
@cdrage updated |
| // placement: | ||
| placement := make(map[string]string) | ||
| for _, j := range composeServiceConfig.Deploy.Placement.Constraints { | ||
| p := strings.Split(j, " == ") |
There was a problem hiding this comment.
missed this, but it's a pain in the butt, but can you test for:
placement:
constraints: [node.role == manager]
as well, as per: https://docs.docker.com/compose/compose-file/#replicas
There was a problem hiding this comment.
@cdrage , added functional test for the same example and it's working
surajnarwade
force-pushed
the
placementv3
branch
from
879e6f4
to
303a922
Compare
|
@surajnarwade Alright, code LGTM (thanks for adding another functional test). Fix conflicts and then it should be good to go. |
surajnarwade
force-pushed
the
placementv3
branch
from
303a922
to
5161a9b
Compare
|
@cdrage fixed conflicts |
pkg/loader/compose/v3.go
Outdated
| } else if p[0] == "engine.labels.operatingsystem" { | ||
| placement["beta.kubernetes.io/os"] = p[1] | ||
| } else { | ||
| log.Warn(p[0], " constraints in placement is not supported") |
There was a problem hiding this comment.
So just a second review, but maybe we should add that only node.hostname and engine.labels.operatingsystem is only supported as a constraint in the warning.
script/test/cmd/tests.sh
Outdated
| @@ -574,5 +597,7 @@ cmd="kompose convert --provider=openshift --stdout -j -f $KOMPOSE_ROOT/examples/ | |||
| sed -e "s;%VERSION%;$version;g" -e "s;%CMD%;$cmd;g" $KOMPOSE_ROOT/script/test/fixtures/examples/output-gitlab-os.json > /tmp/output-os.json | |||
| convert::expect_success_and_warning "kompose convert --provider=openshift --stdout -j -f $KOMPOSE_ROOT/examples/docker-gitlab.yaml" "/tmp/output-os.json" | |||
|
|
|||
|
|
|||
|
|
|||
it will map `engine.labels.operatingsystem` to `beta.kubernetes.io/os` and `node.hostname` to `kubernetes.io/hostname` and all other constraints will not be supported.
surajnarwade
force-pushed
the
placementv3
branch
from
5161a9b
to
35198cc
Compare
|
LGTM |
it will map
engine.labels.operatingsystemtobeta.kubernetes.io/osandnode.hostnametokubernetes.io/hostnameand all other constraints will not be supported.