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

Podex handling multiple images #1898

Merged
merged 1 commit into from
Oct 22, 2014
Merged

Conversation

jhadvig
Copy link
Contributor

@jhadvig jhadvig commented Oct 20, 2014

Podex can now handle multiple images.
Also adding podID flag, with which user can specify the pod ID.
eg:
podex -json -podID=nodejs dockerfile/redis google/nodejs-hello

In case the command will take only one image the podID is optional and image base-name will be used for as a podID

@@ -40,10 +40,11 @@ import (
"gopkg.in/v1/yaml"
)

const usage = "usage: podex [-json|-yaml] <repo/dockerimage>"
const usage = "usage: podex [-json|-yaml] -podID=ID <repo/dockerimage1> ... <repo/dockerimageN>"
Copy link
Contributor

Choose a reason for hiding this comment

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

what about -id should be obvious since the output is a pod configuration..

@proppy
Copy link
Contributor

proppy commented Oct 21, 2014

Just some nits, thanks a lot for doing this :)

@jhadvig
Copy link
Contributor Author

jhadvig commented Oct 21, 2014

@proppy thanks for review. Updated and rebased the PR :)

if *podName == "" {
if flag.NArg() > 1 {
log.Fatal(usage)
} else {
Copy link
Contributor

Choose a reason for hiding this comment

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

you can remove the else, since the log is fatal.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yep, sorry for that

@jhadvig jhadvig force-pushed the podex_images branch 3 times, most recently from 9ded97a to 4f5331e Compare October 21, 2014 23:33
@jhadvig
Copy link
Contributor Author

jhadvig commented Oct 21, 2014

@proppy rebased, sorry for that :)

@@ -120,3 +128,14 @@ func main() {
os.Stdout.Write(bs)
}
}

// Function takes images name which is parsed.
Copy link
Contributor

Choose a reason for hiding this comment

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

parseDockerImage split a docker image name of the form
[REGISTRYHOST/][USERNAME/]NAME[:TAG]

We should probably add a TODO to handle the TAG

@@ -40,10 +40,11 @@ import (
"gopkg.in/v1/yaml"
)

const usage = "usage: podex [-json|-yaml] <repo/dockerimage>"
const usage = "usage: podex [-json|-yaml] -id=ID <repo/dockerimage1> ... <repo/dockerimageN>"
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this should be: username/image1 username/image2, I think repo refers to username/image

@proppy
Copy link
Contributor

proppy commented Oct 22, 2014

LGTM just some docstring nits

@jhadvig
Copy link
Contributor Author

jhadvig commented Oct 22, 2014

@proppy rebased

proppy added a commit that referenced this pull request Oct 22, 2014
Podex handling multiple images
@proppy proppy merged commit 0e8804e into kubernetes:master Oct 22, 2014
@proppy
Copy link
Contributor

proppy commented Oct 22, 2014

Merged thanks.

@soltysh soltysh mentioned this pull request Oct 22, 2014
jsafrane pushed a commit to jsafrane/kubernetes that referenced this pull request Apr 22, 2024
[release-4.14] OCPBUGS-29924: UPSTREAM: <carry>: openshift-kube-apiserver: add kube-apiserver patches
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants