fix circular input/output detection

[bparees]

need to actually compare the imagestreams that are being
input/output, not the image represented by the input to
the output.

fixes https://bugzilla.redhat.com/show_bug.cgi?id=1306507

[bparees]

[test]

[bparees]

@smarterclayton @rhcarvalho ptal. the existing circular input/output detection was insufficient, it compared the input as "docker.io/openshift/ruby-22-centos7"(the imageref) to the output of "library/ruby-22-centos7" but really the input is "ruby-22-centos7"(the imagestream) in the sense that that is what triggers the build. It looks like it worked for the "centos" test case basically out of happenstance (both converted to library/centos and matched)

[bparees]

still working on this, ignore for now.

[bparees]

ok good to go again, i think.

two major changes in output behavior here:

1. the warning (if you specify a bad --to) is the last thing printed, not the first. I think this is better anyway.
2. if there's an error, you still get all the descriptive output first, prior to the error.. I don't love this but i'm also not that hung up about it. The last thing you see is the error, which is what is most important imho.

I haven't spent a ton of time thinking about other approaches, but basically i think this error checking needs to come last (after we've resolved everything) so avoiding the other output until we can do the validation seems like it might be a challenge given the current code structure.

example output:

$ oc new-build -D $'FROM openshift/ruby-22-centos7\nUSER default\nEXPOSE 8080'  --dry-run 
--> Found Docker image c5bd303 (4 months old) from Docker Hub for "openshift/ruby-22-centos7"

    Ruby 2.2 
    -------- 
    Platform for building and running Ruby 2.2 applications

    Tags: builder, ruby, ruby22

    * An image stream will be created as "ruby-22-centos7:latest" that will track the source image
    * A Docker build using a predefined Dockerfile will be created
      * The resulting image will be pushed to image stream "ruby-22-centos7:latest"
      * Every time "ruby-22-centos7:latest" changes a new build will be triggered

error: the input and output image stream tags are identical ("p3/ruby-22-centos7:latest"), please specify a different output reference with --to

(in fact i'll even proactively argue that the additional descriptive "here's what we're doing" information could be useful in understanding the error being reported. So i'm going to claim this is a feature)

[smarterclayton]

The message is too long -

error: the input and output image stream tags are identical ("p3/ruby-22-centos7:latest"), please specify a different output reference with --to

can probably be

error: output image of "ruby-22-centos-7" must be different than input, set a different tag with --to

or similar

[bparees]

it's still kinda long, but changed to:
error: output image of "default/centos:latest" must be different than input, set a different tag with --to

[bparees]

[test]

[rhcarvalho]

Need to document what the argument objects is (a "list" of objects that might contain build configs).

[bparees]

i don't see how that documentation adds value nor it is it the kind of thing we tend to document anywhere else. It's clear from the type what objects is, and we don't document it anywhere else that we take an app.Objects parameter in a function (AddRoutes, AddServices). Not to mention it's a private function so it's not even like it's an api we expect people to consume. The only use is within this package.

And "might contain a buildconfig" is implied by the fact that the function says it's checking the builds, and in the future it might expand to check other things (non-buildconfig related) anyway.

I will add the comment in this case since you've requested it and i'm updating the file anyway, but in general this doesn't seem like a valuable change to make people iterate on.

[smarterclayton]

Yeah generally we'd define how we process the parameter, but we don't discuss the type of the parameter when it's obvious from observation.

[rhcarvalho]

I think just because something is not exported is not an excuse to not document or make things clear.
And if something is missing elsewhere, it doesn't mean we cannot do better.

AFAIK, we're not generating "readable" godocs like one would read for the standard library or other libs we use, but I believe all devs are consuming those docs straight from code every day.

The type is explicit, we don't need to repeat ourselves, but intentions are not always obvious.

For instance, when reading this piece I first wondered why it operates on app.Objects and not on a single api.BuildConfig. It takes extra leaps of thought and clicking, digging, etc, to figure out what is app.Objects and what it actually might contain.

I thought having this type of discussion was beneficial to a code review. After all, we're all going to maintain this code later.

[openshift-bot]

Evaluated for origin test up to e970008

[openshift-bot]

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

[bparees]

failures are unrelated to this change. the newapp tests passed.

[smarterclayton]

LGTM

On Tue, Feb 16, 2016 at 5:55 PM, Ben Parees notifications@github.com
wrote:

failures are unrelated to this change. the newapp tests passed.

—
Reply to this email directly or view it on GitHub
#7321 (comment).

[bparees]

[merge]

[openshift-bot]

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

[openshift-bot]

Evaluated for origin merge up to e970008