Issue 7555 - fixed importimage #7581

soltysh · 2016-02-24T10:10:26Z

@smarterclayton I've made slight modifications in the image-import command to address problems from the issue. Still there are some others I've marked as TODO for now. ptal

0xmichalis · 2016-02-24T10:13:39Z

pkg/cmd/cli/cmd/importimage.go

+				// TODO soltysh: what about the case where specs are only on tags?
+				if len(stream.Spec.DockerImageRepository) == 0 {
+					return fmt.Errorf("flag --all is applicable only to images with .spec.dockerImageRepository defined")
+				} else {


smarterclayton · 2016-02-24T18:27:49Z

pkg/cmd/cli/cmd/importimage.go


 		if all {
 			// importing a whole repository
+			// TODO soltysh: write tests to cover all the possible usecases!!!


Add a test now that covers the old behavior and new.

soltysh · 2016-02-24T20:24:39Z

@smarterclayton I've addressed the dot comment. I don't feel comfortable with import image not being tested at all, so I agree with your comment. I'll work on creating extensive tests of the ImageStreamImport created in imageimport.go as a followup if you don't mind.

smarterclayton · 2016-02-24T21:26:23Z

Even a test-cmd test would be fine now. I'd be happy with 2 or 3 more "import-image" commands added to images.sh

soltysh · 2016-02-26T14:13:01Z

@smarterclayton I've reworked the image-import command to follow Complete-Validate-Run scheme and split it into testable chunks (commit 1). The bugfix and tests are in 2nd commit. ptal

soltysh · 2016-02-26T14:13:11Z

[test]

soltysh · 2016-02-26T16:27:01Z

Fixed gofmt problem.

soltysh · 2016-02-27T23:17:12Z

One clean run, one flake filled new one for that. @smarterclayton still waiting your signoff.

smarterclayton · 2016-02-28T00:12:17Z

Will review on Monday

smarterclayton · 2016-02-29T14:50:31Z

pkg/cmd/cli/cmd/importimage.go

 		Run: func(cmd *cobra.Command, args []string) {
-			err := RunImportImage(f, out, cmd, args)
-			kcmdutil.CheckErr(err)
+			if err := opts.Complete(f, args, out); err != nil {


Don't force these to be usage errors, return usage errors and call CheckErr directly.

Right, Complete doesn't deal with usage errors here, the only ones there are from getting default namespace or clients.

The pattern here should be cmdutil.CheckErr(opts.Complete()) and the same for the others. Validate should return usage errors as necessary.

I see, I must have been looking at older commands before this new pattern. Will change.

smarterclayton · 2016-02-29T14:59:52Z

Change looks ok, can we add a few more branch tests to hack/test-cmd.sh

soltysh · 2016-02-29T15:05:36Z

@smarterclayton what kind of tests are you thinking about?

soltysh · 2016-02-29T16:07:58Z

@smarterclayton I've fixed this one issue, I'll be working on the tests both unit and cmd as a followup. I'd like to add --dry-run you've mentioned as well.

soltysh · 2016-02-29T16:08:27Z

Let's have this one merged if you don't mind.

smarterclayton · 2016-02-29T16:23:21Z

I'd like additional test-cmd tests here before we merge this that work before and after.

soltysh · 2016-03-01T09:53:30Z

I'd like additional test-cmd tests here before we merge this that work before and after.

Refactored code but without my fix, lines 289-293:

if o.All {
    // importing a whole repository
    if len(from) == 0 {
        from = o.Target
    }

Covers the usecase where an image stream is found and we want to import all tags, without from being passed: oc import-image myis --all. This sets from to be the name of the image stream passed as an argument, which looking down the code is then being passed to importer as:

kapi.ObjectReference{
    Kind: "DockerImage",
    Name: from,
},

Which will generally fail when trying to read image metadata from remote repository. The only use-case where this will work is when your image stream uses the name of the images provided by docker hub (eg. mysql, python, etc.)

Refactored code but without my fix, lines 337-348:

} else {
    // create a new tag
    if len(from) == 0 {
        from = o.Target
    }

Covers the usecase where we import single tag, from existing image stream, but we omit the tag name, assuming default latest will be applied: oc import-image myis. Since the tag is not explicitly specified in the image stream's spec we fail to find it and we fall in the above else case where we again set from to point to image stream name. From this definition we create a latest tag pointing
to:

kapi.ObjectReference{
    Kind: "DockerImage",
    Name: from,
},

Further more we set above ObjectReference in the last else as main image stream's
From, which is not necessarily correct either.

@smarterclayton having said that I'm not sure there was correct behavior before, that I can actually address with tests. I'm ok with adding more tests and I'm currently working on, it but I don't see any before that was actually working, though maybe I'm missing something here. If so please point me to the before usecase you're talking about.

edit: updated code links to current commit

…plit the code so it's testable + added tests.

soltysh · 2016-03-01T11:17:26Z

@smarterclayton I've addressed your comment and added more tests, ptal

…ec for images that failed previous import.

soltysh · 2016-03-01T12:31:01Z

Of course I forgot to update tests after changing Validate - fixed.

soltysh · 2016-03-01T14:22:32Z

It looks like flake from #6077

soltysh · 2016-03-01T15:39:56Z

@smarterclayton comment addressed

soltysh · 2016-03-01T20:44:03Z

@smarterclayton test with checking no of imported tags. For your question why I've changed from importing mysql to docker.io/mysql is to remove ambiguity for having two mysql, where one is pointing to our image stream and second to hub's image.

openshift-bot · 2016-03-01T20:45:31Z

Evaluated for origin test up to 97b0f7f

smarterclayton · 2016-03-01T20:56:09Z

Lgtm [merge]

openshift-bot · 2016-03-01T22:13:58Z

continuous-integration/openshift-jenkins/test FAILURE (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/1754/)

openshift-bot · 2016-03-02T08:50:14Z

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_requests_origin/5165/) (Image: devenv-rhel7_3586)

soltysh · 2016-03-02T09:37:40Z

Test flake from #7662, merge flake looks like #6077.
re-[merge]

openshift-bot · 2016-03-02T09:40:16Z

Evaluated for origin merge up to 97b0f7f

Merged by openshift-bot

0xmichalis reviewed Feb 24, 2016
View reviewed changes

soltysh force-pushed the issue7555 branch from a984560 to 9ab3335 Compare February 24, 2016 10:31

smarterclayton reviewed Feb 24, 2016
View reviewed changes

soltysh force-pushed the issue7555 branch from 9ab3335 to bf0c0df Compare February 24, 2016 20:22

soltysh force-pushed the issue7555 branch from bf0c0df to a56313e Compare February 26, 2016 14:11

soltysh mentioned this pull request Feb 26, 2016

Registry daemonset #7596

Merged

soltysh force-pushed the issue7555 branch from a56313e to 5348a55 Compare February 26, 2016 16:21

soltysh mentioned this pull request Feb 26, 2016

flake: Unable to perform initial IP allocation check #7662

Closed

smarterclayton reviewed Feb 29, 2016
View reviewed changes

soltysh force-pushed the issue7555 branch from 5348a55 to 5aeaa63 Compare February 29, 2016 16:07

Refactor import-image to Complete-Validate-Run scheme. Additionally s…

0db7669

…plit the code so it's testable + added tests.

soltysh force-pushed the issue7555 branch from 5aeaa63 to b3e10c2 Compare March 1, 2016 11:13

Issue 7555 - fixed importimage which was picking wrong docker pull sp…

1b8c4af

…ec for images that failed previous import.

soltysh force-pushed the issue7555 branch from b3e10c2 to 78a7bc0 Compare March 1, 2016 12:30

soltysh force-pushed the issue7555 branch from 78a7bc0 to c3ce27b Compare March 1, 2016 15:39

Added more cmd tests for import-image to cover main branches

97b0f7f

soltysh force-pushed the issue7555 branch from c3ce27b to 97b0f7f Compare March 1, 2016 20:41

openshift-bot pushed a commit that referenced this pull request Mar 2, 2016

Merge pull request #7581 from soltysh/issue7555

2eb8022

Merged by openshift-bot

openshift-bot merged commit 2eb8022 into openshift:master Mar 2, 2016

soltysh deleted the issue7555 branch March 3, 2016 07:38

Issue 7555 - fixed importimage #7581

Issue 7555 - fixed importimage #7581

Uh oh!

Conversation

soltysh commented Feb 24, 2016

Uh oh!

0xmichalis Feb 24, 2016

Choose a reason for hiding this comment

Uh oh!

soltysh Feb 24, 2016

Choose a reason for hiding this comment

Uh oh!

smarterclayton Feb 24, 2016

Choose a reason for hiding this comment

Uh oh!

soltysh commented Feb 24, 2016

Uh oh!

smarterclayton commented Feb 24, 2016

Uh oh!

soltysh commented Feb 26, 2016

Uh oh!

soltysh commented Feb 26, 2016

Uh oh!

soltysh commented Feb 26, 2016

Uh oh!

soltysh commented Feb 27, 2016

Uh oh!

smarterclayton commented Feb 28, 2016 via email

Uh oh!

smarterclayton Feb 29, 2016

Choose a reason for hiding this comment

Uh oh!

soltysh Feb 29, 2016

Choose a reason for hiding this comment

Uh oh!

smarterclayton Feb 29, 2016 via email

Choose a reason for hiding this comment

Uh oh!

soltysh Mar 1, 2016

Choose a reason for hiding this comment

Uh oh!

smarterclayton commented Feb 29, 2016

Uh oh!

soltysh commented Feb 29, 2016

Uh oh!

soltysh commented Feb 29, 2016

Uh oh!

soltysh commented Feb 29, 2016

Uh oh!

smarterclayton commented Feb 29, 2016 via email

Uh oh!

soltysh commented Mar 1, 2016

Uh oh!

soltysh commented Mar 1, 2016

Uh oh!

soltysh commented Mar 1, 2016

Uh oh!

soltysh commented Mar 1, 2016

Uh oh!

soltysh commented Mar 1, 2016

Uh oh!

soltysh commented Mar 1, 2016

Uh oh!

openshift-bot commented Mar 1, 2016

Uh oh!

smarterclayton commented Mar 1, 2016 via email

Uh oh!

openshift-bot commented Mar 1, 2016

Uh oh!

openshift-bot commented Mar 2, 2016

Uh oh!

soltysh commented Mar 2, 2016

Uh oh!

openshift-bot commented Mar 2, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone