Removed fields added to the build strategy when using --strategy=docker. #9553

oatmealraisin · 2016-06-24T19:26:23Z

Old output:

$ oc new-app https://github.com/openshift/ruby-hello-world --strategy=docker -o yaml
...
- apiVersion: v1
  kind: BuildConfig
  metadata:
    annotations:
      openshift.io/generated-by: OpenShiftNewApp
    creationTimestamp: null
    labels:
      app: ruby-hello-world
    name: ruby-hello-world
  spec:
    output:
      to:
        kind: ImageStreamTag
        name: ruby-hello-world:latest
    postCommit: {}
    resources: {}
    source:
      git:
        uri: https://github.com/openshift/ruby-hello-world
      secrets: []
      type: Git
    strategy:
      dockerStrategy:
        from:
          kind: ImageStreamTag
          name: ruby-22-centos7:latest
      type: Docker
    triggers:
    - github:
        secret: 9kT3d_Dtngco-2amnzkE
      type: GitHub
    - generic:
        secret: b5HW8CYUDQqVK7UgTO7F
      type: Generic
    - type: ConfigChange
    - imageChange: {}
      type: ImageChange
  status:
    lastVersion: 0
...

New Output:

$ oc new-app https://github.com/openshift/ruby-hello-world --strategy=docker -o yaml
...
- apiVersion: v1
  kind: BuildConfig
  metadata:
    annotations:
      openshift.io/generated-by: OpenShiftNewApp
    creationTimestamp: null
    labels:
      app: ruby-hello-world
    name: ruby-hello-world
  spec:
    output:
      to:
        kind: ImageStreamTag
        name: ruby-hello-world:latest
    postCommit: {}
    resources: {}
    source:
      git:
        uri: https://github.com/openshift/ruby-hello-world
      secrets: []
      type: Git
    strategy:
      dockerStrategy: {}
      type: Docker
    triggers:
    - github:
        secret: ue-VeLrxT1w_OnBpdQhp
      type: GitHub
    - generic:
        secret: oS3Jr_kkqLvLPQJqsntn
      type: Generic
    - type: ConfigChange
    - imageChange: {}
      type: ImageChange
  status:
    lastVersion: 0
...

oatmealraisin · 2016-06-24T19:26:56Z

@bparees @csrwng ptal

oatmealraisin · 2016-06-24T19:40:49Z

Rebased

oatmealraisin · 2016-06-24T20:02:56Z

[test]

bparees · 2016-06-24T20:06:52Z

I think the buildconfig you've produced is invalid because it has an ImageChangeTrigger defined, but there is no image for the ICT to trigger from. That should result in a validation error if you actually let new-app try to create these objects:
https://github.com/openshift/origin/blob/master/pkg/build/api/validation/validation.go#L561-L568

I also think is solution is overly aggressive, for example it prevents me from doing:
oc new-app ruby~https://github.com/openshift/ruby-hello-world --strategy=docker

which i might want to do if i definitely do want the FROM line in my Dockerfile to be overwritten by a particular image that i'm basing my build on. As was noted here: #6347 (comment)

rhcarvalho · 2016-06-28T08:04:59Z

@oatmealraisin thanks for taking a look at this! Suggestions:

The new-app code base is complicated, try to start with a test case to nail down the issue. As @bparees noted, we need to distinguish the two cases -- with and without an explicit builder image in the arguments. Find me on IRC if you need help with the tests :)
A common practice is to mention the issue in the PR description (already there, good), do not mention it in the commit message: as the PR evolves, every time you rebase your commits the issue page gets a new notification about the commit.

oatmealraisin · 2016-07-13T19:09:37Z

I added some logic to check if we got the image from a Dockerfile. @bparees ptal?

Output from [source]:

kind: BuildConfig
  metadata:
    annotations:
      openshift.io/generated-by: OpenShiftNewApp
    creationTimestamp: null
    labels:
      app: ruby-hello-world
    name: ruby-hello-world
  spec:
    output:
      to:
        kind: ImageStreamTag
        name: ruby-hello-world:latest
    postCommit: {}
    resources: {}
    source:
      git:
        uri: https://github.com/openshift/ruby-hello-world
      secrets: []
      type: Git
    strategy:
      dockerStrategy: {}
      type: Docker
    triggers:
    - github:
        secret: 8_gKQQDiSQZonpmb8c-J
      type: GitHub
    - generic:
        secret: afQxGkdXB8Izd6zmIi1c
      type: Generic
    - type: ConfigChange
  status:
    lastVersion: 0

Output from [image]~[source]:

  kind: BuildConfig
  metadata:
    annotations:
      openshift.io/generated-by: OpenShiftNewApp
    creationTimestamp: null
    labels:
      app: ruby-hello-world
    name: ruby-hello-world
  spec:
    output:
      to:
        kind: ImageStreamTag
        name: ruby-hello-world:latest
    postCommit: {}
    resources: {}
    source:
      git:
        uri: https://github.com/openshift/ruby-hello-world
      secrets: []
      type: Git
    strategy:
      dockerStrategy:
        from:
          kind: ImageStreamTag
          name: origin:latest
      type: Docker
    triggers:
    - github:
        secret: BaMQLeLuFW4ZhNJ8wwF_
      type: GitHub
    - generic:
        secret: r3dMynXk8HuW7UlyOwdf
      type: Generic
    - type: ConfigChange
    - imageChange: {}
      type: ImageChange
  status:
    lastVersion: 0

bparees · 2016-07-13T20:11:33Z

pkg/generate/app/componentref.go

-	ScratchImage  bool
+	ExpectToBuild  bool
+	ScratchImage   bool
+	FromDockerfile bool


since you were complaining about comments before, comment your new field :)

bparees · 2016-07-13T20:12:10Z

lgtm but please add some tests that cover the different behaviors so we can confirm we don't regress this behavior later.

oatmealraisin · 2016-07-14T16:35:19Z

Added some tests, hopefully they pass this time!

oatmealraisin · 2016-07-14T18:09:06Z

[test]

rhcarvalho · 2016-07-14T18:48:27Z

pkg/generate/app/componentref.go

@@ -299,6 +302,9 @@ type ComponentInput struct {

 	ExpectToBuild bool
 	ScratchImage  bool
+	// For keeping track of if this image was retrieved from a Dockerfile,
+	// which changes our BuildConfig results


I'm a bit confused, is this about a Dockerfile or about a reference to a Docker image?

In oc new-app php~https://github.com/org/repo, php refers to a Docker image, not a Dockerfile.

@rhcarvalho in your scenario, FromDockerfile would be false.

FromDockerfile is true when we figured out which image you want to use by parsing the Dockerfile you supplied (either in the repo or via the -D flag).

@oatmealraisin it would be better if this comment explained how it changes the results. "when true, an imagechangetrigger will not be associated with the buildconfig because the user has not expressed a desire to reference an imagestream, they are directly referencing an image" or some such thing.

Aha, so it doesn't relate to that syntax. Thanks @bparees.
Can't say the flow is absolutely clear to me, but at least the name seems accurate :)

@bparees I was thinking about whether it should explain the intent instead of the idea, I'll change the comment

@rhcarvalho When the input [image]~[source] gets converted into a ComponentReference, and then into an ImageRef, the information about where the image came from gets lost. If we just specify the [source], the image gets filled in from the Dockerfile "FROM". I added a flag so that some information gets preserved, and then I use that information to change the outputted BuildConfig. I really don't like the flow either, it's kinda scotch-tape, but couldn't find a better way to do it. I'm definitely open to suggestions!

it's kinda scotch-tape

👍 💯

but couldn't find a better way to do it. I'm definitely open to suggestions!

I think the approach you described is perfectly valid, no concerns there.

stevekuznetsov · 2016-07-14T21:08:11Z

https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/6297/ flaked with #9371

oatmealraisin · 2016-07-14T21:20:57Z

[test]

oatmealraisin · 2016-07-19T17:49:12Z

re[test]

oatmealraisin · 2016-07-19T17:59:12Z

rebased

oatmealraisin · 2016-07-19T18:41:28Z

Changed the comment, @bparees @rhcarvalho ptal?

Also, I've added a test to newapp.sh, should I add one anywhere else?

bparees · 2016-07-19T20:44:47Z

pkg/generate/app/imageref.go

@@ -140,6 +140,10 @@ type ImageRef struct {
 	OutputImage     bool
 	Insecure        bool
 	HasEmptyDir     bool
+	// For keeping track of if this image was retrieved from a Dockerfile,


proper godoc starts w/ the object name, so "FromDockerfile keeps track of....."

same for the other one above.

bparees · 2016-07-19T20:52:18Z

@oatmealraisin you still need to fix tests which are broken by these changes, the ones you reported as a flake here: #9879

rhcarvalho · 2016-07-20T07:40:24Z

pkg/generate/app/sourcelookup.go

@@ -204,14 +204,17 @@ func (r *SourceRepository) Detect(d Detector, dockerStrategy bool) error {
 	if r.info != nil {
 		return nil
 	}
+


Are the new lines here intentional? It's strange to change this file as part of this PR just for the empty lines.

+1, i'm not averse to introducing whitespace if you're already touching the file anyway, but it does tend to cross a line if you're not making any other changes.

oatmealraisin · 2016-07-20T13:35:24Z

re[test]

oatmealraisin · 2016-07-25T17:10:22Z

re[test]

oatmealraisin · 2016-08-25T20:19:52Z

@bparees I updated some tests to prevent regression, and also removed tests that were invalidated because of this change. ptal?

bparees · 2016-08-25T21:34:42Z

@oatmealraisin we can't just remove the tests/checks for warnings, we need to create equivalent ones that will still test the circular build warning logic for scenarios under which it can still happen.

bparees · 2016-09-20T08:06:24Z

@oatmealraisin bump

oatmealraisin · 2016-11-04T23:13:59Z

@bparees I've modified the tests to trigger the proper responses, but one of the tests relies on the input repository having the same name as the requested builder image (openshift/ruby-20-centos7~https://github.com/oatmealraisin/ruby-20-centos7). I've used my own repository to do this, but we probably want something with the openshift account?

This allows us to check later in new-app whether an Image needs a ImageChangeTrigger, or whether we should continue to get it from the Dockerfile it was found in.

oatmealraisin · 2016-11-05T13:17:08Z

Flake #11452, [test]

openshift-bot · 2016-11-05T13:21:24Z

Evaluated for origin test up to 834ce8d

openshift-bot · 2016-11-05T14:53:26Z

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/11178/) (Base Commit: 046fad4)

bparees · 2016-11-06T17:21:22Z

@oatmealraisin i apologize because i know you've spent a ton of time on this but with the benefit of hindsight, I think the original issue should be closed (in fact i have closed it) rather than implementing what it proposes. I put my reasoning in the original issue (#6347 (comment))

oatmealraisin · 2016-11-07T16:30:07Z

Roger that!

bparees reviewed Jul 13, 2016
View reviewed changes

rhcarvalho reviewed Jul 14, 2016
View reviewed changes

bparees reviewed Jul 19, 2016
View reviewed changes

rhcarvalho reviewed Jul 20, 2016
View reviewed changes

openshift-bot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Aug 12, 2016

bparees added this to the 1.3.1 milestone Aug 25, 2016

smarterclayton modified the milestones: 1.3.1, 1.4.0 Sep 19, 2016

smarterclayton modified the milestones: 1.4.0, 1.3.1 Sep 19, 2016

Added a FromDockerfile boolean to ImageRef

834ce8d

This allows us to check later in new-app whether an Image needs a ImageChangeTrigger, or whether we should continue to get it from the Dockerfile it was found in.

oatmealraisin closed this Nov 7, 2016

Removed fields added to the build strategy when using --strategy=docker. #9553

Removed fields added to the build strategy when using --strategy=docker. #9553

Conversation

oatmealraisin commented Jun 24, 2016

oatmealraisin commented Jun 24, 2016

oatmealraisin commented Jun 24, 2016

oatmealraisin commented Jun 24, 2016

bparees commented Jun 24, 2016

rhcarvalho commented Jun 28, 2016

oatmealraisin commented Jul 13, 2016

Choose a reason for hiding this comment

bparees commented Jul 13, 2016

oatmealraisin commented Jul 14, 2016

oatmealraisin commented Jul 14, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rhcarvalho Jul 17, 2016 • edited Loading

Choose a reason for hiding this comment

stevekuznetsov commented Jul 14, 2016

oatmealraisin commented Jul 14, 2016

oatmealraisin commented Jul 19, 2016

oatmealraisin commented Jul 19, 2016

oatmealraisin commented Jul 19, 2016

Choose a reason for hiding this comment

bparees commented Jul 19, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oatmealraisin commented Jul 20, 2016

oatmealraisin commented Jul 25, 2016

oatmealraisin commented Aug 25, 2016

bparees commented Aug 25, 2016

bparees commented Sep 20, 2016

oatmealraisin commented Nov 4, 2016

oatmealraisin commented Nov 5, 2016

openshift-bot commented Nov 5, 2016

openshift-bot commented Nov 5, 2016

bparees commented Nov 6, 2016

oatmealraisin commented Nov 7, 2016

rhcarvalho Jul 17, 2016 •

edited

Loading