Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

error out if controller object is specified with "restart: on-failure" #373

Merged
merged 1 commit into from
Jan 30, 2017
Merged

error out if controller object is specified with "restart: on-failure" #373

merged 1 commit into from
Jan 30, 2017

Conversation

procrypt
Copy link

Fixes #354
CC: @kadel @surajssd

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jan 11, 2017
@procrypt procrypt force-pushed the error_should_be_displayed branch from 0dee862 to 83723f8 Compare January 11, 2017 11:33
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.02%) to 46.983% when pulling 83723f8 on procrypt:error_should_be_displayed into 7dbf00c on kubernetes-incubator:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.02%) to 46.983% when pulling 83723f8 on procrypt:error_should_be_displayed into 7dbf00c on kubernetes-incubator:master.

kadel
kadel requested changes Jan 11, 2017
View reviewed changes
Copy link
Member

@kadel kadel left a comment

Choose a reason for hiding this comment

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

This is not working.
with docker-compose file from #354

▶ ./kompose convert         
FATA[0000] Controller object cannot be specified with restart: 'on-failure'

this should be valid

kadel
kadel reviewed Jan 11, 2017
View reviewed changes
pkg/transformer/kubernetes/kubernetes.go
@@ -463,6 +463,10 @@ func (k *Kubernetes) Transform(komposeObject kobject.KomposeObject, opt kobject.
service := komposeObject.ServiceConfigs[name]
var objects []runtime.Object

// Show error if no controller object is specified and restart: "on-failure"
if opt.CreateD && service.Restart == "on-failure" || opt.CreateDS && service.Restart == "on-failure" || opt.CreateDeploymentConfig && service.Restart == "on-failure" || opt.CreateRC && service.Restart == "on-failure" {
Copy link
Member

Choose a reason for hiding this comment

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

this doesn't look right :-(
It should be also handled somewhere else. Don't forget about OpenShift transformer ;-)

Copy link
Member

Choose a reason for hiding this comment

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

k8sutils.go would be a better area. similar to your work on cpu_shares https://github.com/kubernetes-incubator/kompose/pull/294/files

kadel
kadel reviewed Jan 11, 2017
View reviewed changes
script/test/cmd/tests.sh
@@ -111,7 +111,7 @@ convert::expect_success_and_warning "kompose --bundle $KOMPOSE_ROOT/script/test/
# Test related to restart options in docker-compose
# kubernetes test
convert::expect_success "kompose -f $KOMPOSE_ROOT/script/test/fixtures/restart-options/docker-compose-restart-no.yml convert --stdout -j" "$KOMPOSE_ROOT/script/test/fixtures/restart-options/output-k8s-restart-no.json"
convert::expect_success "kompose -f $KOMPOSE_ROOT/script/test/fixtures/restart-options/docker-compose-restart-onfail.yml convert --stdout -j" "$KOMPOSE_ROOT/script/test/fixtures/restart-options/output-k8s-restart-onfail.json"
Copy link
Member

Choose a reason for hiding this comment

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

I remove this test? It is valid

Copy link
Member

Choose a reason for hiding this comment

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

new failing tests should be added, which will check if it fails on adding more objects on convert.

cdrage
cdrage requested changes Jan 11, 2017
View reviewed changes
pkg/transformer/kubernetes/kubernetes.go
@@ -463,6 +463,10 @@ func (k *Kubernetes) Transform(komposeObject kobject.KomposeObject, opt kobject.
service := komposeObject.ServiceConfigs[name]
var objects []runtime.Object

// Show error if no controller object is specified and restart: "on-failure"
if opt.CreateD && service.Restart == "on-failure" || opt.CreateDS && service.Restart == "on-failure" || opt.CreateDeploymentConfig && service.Restart == "on-failure" || opt.CreateRC && service.Restart == "on-failure" {
Copy link
Member

Choose a reason for hiding this comment

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

k8sutils.go would be a better area. similar to your work on cpu_shares https://github.com/kubernetes-incubator/kompose/pull/294/files

@procrypt procrypt force-pushed the error_should_be_displayed branch 2 times, most recently from 30355e5 to 39e2898 Compare January 16, 2017 12:37
@procrypt
Copy link
Author

@kadel @surajssd please review.

cdrage
cdrage requested changes Jan 20, 2017
View reviewed changes
Copy link
Member

@cdrage cdrage left a comment

Choose a reason for hiding this comment

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

Thank for refactoring your original code! This looks a lot cleaner :)

cmd/convert.go
IsDeploymentFlag: cmd.Flags().Lookup("deployment").Changed,
IsDaemonSetFlag: cmd.Flags().Lookup("daemon-set").Changed,
IsReplicationControllerFlag: cmd.Flags().Lookup("replication-controller").Changed,
IsDeploymentConfigFlag: cmd.Flags().Lookup("deployment-config").Changed,
Copy link
Member

Choose a reason for hiding this comment

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

👍 on all above :)

pkg/transformer/kubernetes/kubernetes.go
// Error out if Controller Object is specified with restart: 'on-failure'
if opt.IsDeploymentFlag || opt.IsDaemonSetFlag || opt.IsReplicationControllerFlag {
logrus.Fatalf("Controller object cannot be specified with restart: 'on-failure'")
}
Copy link
Member

Choose a reason for hiding this comment

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

Don't really understand this part. If a user doesn't pass in these flags, it won't error out?

Copy link
Author

Choose a reason for hiding this comment

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

@cdrage as @kadel said, kompose convert should be valid if the user don't provide any flag, but it should fail it the user has provided any flag to it.

pkg/transformer/openshift/openshift.go
// Error out if Controller Object is specified with restart: 'on-failure'
if opt.IsDeploymentConfigFlag {
logrus.Fatalf("Controller object cannot be specified with restart: 'on-failure'")
}
Copy link
Member

Choose a reason for hiding this comment

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

same here.. What happens if the user doesnt pass in the flag but still supplies an "on-failure"?

Copy link
Author

@procrypt procrypt Jan 24, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

@cdrage If the user don't pass the flag and still supplies on-failure, then kompose convert will create the config files for it.

@cdrage
Copy link
Member

cdrage commented Jan 24, 2017

Thanks for the comments. This LGTM :) All green from me.

@procrypt
Copy link
Author

Image

@procrypt procrypt force-pushed the error_should_be_displayed branch from 39e2898 to 1ecd32a Compare January 30, 2017 12:23
@cdrage
Copy link
Member

cdrage commented Jan 30, 2017

@procrypt Thanks for the awesome work! Merging away :)

@cdrage cdrage merged commit 2f05ecd into kubernetes:master Jan 30, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kadel kadel kadel requested changes

@surajssd surajssd surajssd left review comments

@cdrage cdrage cdrage approved these changes

Assignees
No one assigned
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@procrypt @coveralls @cdrage @kadel @surajssd @k8s-ci-robot