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.

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

Validate dockerfilepath in buildconfig #609

Merged
merged 1 commit into from
May 31, 2017

Conversation

surajnarwade
Copy link
Contributor

This PR will resolve #594 by validating dockerfilepath based on whether
it is relative path or not.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label May 18, 2017
@surajnarwade surajnarwade force-pushed the validate_dockerfilepath branch from d86feee to 08b8198 Compare May 18, 2017 08:26
@surajnarwade
Copy link
Contributor Author

cc @cdrage @kadel @surajssd needs review

@kadel
Copy link
Member

kadel commented May 22, 2017

hmm, I can't make it work :-(

version: "2"

services:
    foo:
        build:
          context: "./build"
          dockerfile: /Dockerfile
        image: "tomaskral/foobar"
        command: "sleep 3600"

output:

▶ kompose --provider openshift convert --stdout
apiVersion: v1
items: null
kind: List
metadata: {}

@cdrage
Copy link
Member

cdrage commented May 23, 2017

@surajnarwade same issue as @kadel

github.com/kubernetes-incubator/kompose  pr_609 ✔                                                                                                                                                                                                                            7d  
▶ ./kompose --provider=openshift convert --stdout 
apiVersion: v1
items: null
kind: List
metadata: {}

@surajnarwade surajnarwade force-pushed the validate_dockerfilepath branch from 08b8198 to f5899b8 Compare May 24, 2017 06:43
@surajnarwade
Copy link
Contributor Author

@cdrage @kadel, can you please try now, i have updated the PR

@surajssd
Copy link
Member

@ashetty1 can you see if this PR works for you and solve your issue then we can go ahead with it, otherwise the code LGTM, but if @ashetty1 gives a +1 we can merge it, also @surajnarwade can you please also write command line tests?

@@ -330,6 +330,11 @@ func (c *Compose) LoadFile(files []string) (kobject.KomposeObject, error) {
envs := loadEnvVars(composeServiceConfig.Environment)
serviceConfig.Environment = envs

//Validate dockerfile path
if filepath.IsAbs(serviceConfig.Dockerfile) {
log.Fatalf("%q defined in service %q is absolute path, it must be a relative path.", serviceConfig.Dockerfile, name)
Copy link
Member

Choose a reason for hiding this comment

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

Grammar error.

%q is an absolute path is better.

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.

Other than my small note (and a command line test being added), LGTM.

@surajnarwade surajnarwade force-pushed the validate_dockerfilepath branch from f5899b8 to 539d0fa Compare May 30, 2017 10:58
This PR will resolve kubernetes#594 by validating dockerfilepath based on whether
it is relative path or not.
@surajnarwade surajnarwade force-pushed the validate_dockerfilepath branch from 539d0fa to 581e181 Compare May 30, 2017 11:01
@surajnarwade
Copy link
Contributor Author

@cdrage updated PR with required changes,
@ashetty1 , your review needed.

@ashetty1
Copy link
Contributor

LGTM @surajnarwade

@cdrage
Copy link
Member

cdrage commented May 31, 2017

LGTM

@cdrage cdrage merged commit 1f0cf0b into kubernetes:master May 31, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

kompose convert should validate dockerfilepath
6 participants