new-app: appropriately warn/error on circular builds #12104
Conversation
| @@ -0,0 +1,21 @@ | |||
| { | |||
There was a problem hiding this comment.
Yeah I've heard that. Seemed to me that as long as json is supported, we should have a mixture in our various tests (so I intentionally included a bit of both in this change).
How strong is the preference?
There was a problem hiding this comment.
The word I've heard from on high is to just use YAML everywhere. If we want examples of JSON, we can have them in actual examples (under examples/...) but for stuff devs are working with the decision was YAML.
| if len(c.To) == 0 { | ||
| // Output reference was generated, return error. | ||
| return nil, fmt.Errorf("%v, set a different tag with --to", err) | ||
| } | ||
| // Output reference was explicitly provided, print warning. | ||
| fmt.Fprintf(c.ErrOut, "--> WARNING: %v\n", err) | ||
| } else { | ||
| return nil, err | ||
| // circular reference handling for `oc new-app` | ||
| if len(templateObjects) > 0 { |
There was a problem hiding this comment.
@gabemontero it is possible for us to be generating new objects as well as processing template objects. We should check for circular references on the 2 sets of objects separately if we want different behaviors for each.
There was a problem hiding this comment.
Interesting ... so only allow the circular refs if they were induced by the template objects specifically. OK, I'll work on a refactor.
gabemontero
force-pushed
the
newappCircle
branch
from
7c8f428 to
e6849c9
Compare
|
updates pushed ... ptal |
| - apiVersion: v1 | ||
| kind: ImageStream | ||
| metadata: | ||
| creationTimestamp: 2016-12-02T18:55:30Z |
There was a problem hiding this comment.
you should likely get rid of these fields (creationTimestamp, generation, resourceVersion, selfLink, uid)
| selfLink: /oapi/v1/namespaces/myproject/imagestreams/newapp-circular-template | ||
| uid: e5ad1d91-b8c0-11e6-9803-68f7287398a7 | ||
| spec: {} | ||
| status: |
| apiVersion: v1 | ||
| kind: Template | ||
| metadata: | ||
| creationTimestamp: null |
There was a problem hiding this comment.
get rid of creationTimestamp in this file
Bug 1393549 Bugzilla link https://bugzilla.redhat.com/show_bug.cgi?id=1393549 Detailed explanation Citing of cirular builds were performed for new-build but not new-app; With new-app, if template, simply warn, if not template fail command
gabemontero
force-pushed
the
newappCircle
branch
from
e6849c9 to
4c93305
Compare
|
yaml trimmed |
|
LGTM [merge] |
|
[Test]ing while waiting on the merge queue |
|
Evaluated for origin test up to 4c93305 |
|
continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/11957/) (Base Commit: ec171a1) |
|
#12112 |
|
Evaluated for origin merge up to 4c93305 |
|
continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/11988/) (Base Commit: 38e6fba) (Image: devenv-rhel7_5482) |
Bug 1393549
Bugzilla link https://bugzilla.redhat.com/show_bug.cgi?id=1393549
Detailed explanation
Citing of cirular builds were performed for new-build but not new-app;
With new-app, if template, simply warn, if not template fail command
@csrwng PTAL