-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
indicate builds that can't push #3316
indicate builds that can't push #3316
Conversation
Image ImageTagLocation | ||
Build *BuildConfigNode | ||
Image ImageTagLocation | ||
ImageStream *ImageStreamNode |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd really prefer this to be under ImageTagLocation, such that you can ask the imagetaglocation if it is resolved as being valid.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That obviates the need for tracking the image stream separately - it's just a check on whether an outbound edge exists when you create the pipeline.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So would I, but that means that you end up with a method on a shared interface that's only sometimes valid. That's your preference?
Also, why do we have the veneer on the graph. Straight graph traversal would make this point moot.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm thinking more behavior oriented - Resolved() true/false or Exists() true/false
----- Original Message -----
@@ -21,8 +21,9 @@ type DeploymentPipelineMap
map[*DeploymentConfigNode][]ImagePipeline
// ImagePipeline represents a build, its output, and any inputs. The input
// to a build may be another ImagePipeline.
type ImagePipeline struct {
- Image ImageTagLocation
- Build *BuildConfigNode
- Image ImageTagLocation
- ImageStream *ImageStreamNode
So would I, but that means that you end up with a method on a shared
interface that's only sometimes valid. That's your preference?Also, why do we have the veneer on the graph. Straight graph traversal would
make this point moot.
Reply to this email directly or view it on GitHub:
https://github.com/openshift/origin/pull/3316/files#r32769193
9318df5
to
839290d
Compare
initial refactor done, looking at adding the overall check now. |
839290d
to
07b9272
Compare
Updated text in description. |
I'd like to get #3334 and then rebase this. That will fix the imagepruning test. |
07b9272
to
7e432c2
Compare
Rebase pls? |
7e432c2
to
c3c4721
Compare
rebased |
imageStreamFailure := "" | ||
// if we're using an image stream and that image stream is the internal registry and that registry doesn't exist | ||
if (build.Parameters.Output.To != nil) && !pushTargetResolved { | ||
imageStreamFailure = fmt.Sprintf(" (can't push to image)") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you plan to add any varargs or should this just be a plain string?
comments addressed and one bug fixed. |
I don't think I have any additional comments. Ready to test in Jenkins? |
Sure, though the merge task does the same. |
3cb55aa
to
207d2d2
Compare
[test] |
continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin/3562/) |
|
Unhappy jenkins [test] |
Hub flake [test] |
green! |
207d2d2
to
0f7089d
Compare
IsFound bool | ||
} | ||
|
||
func (n ImageStreamNode) Found() bool { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't (yet) used anywhere, is it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't (yet) used anywhere, is it?
Not yet. The missing resource status check requires it.
[merge] |
continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_requests_origin/2687/) (Image: devenv-fedora_1954) |
Evaluated for origin up to 0f7089d |
Fixes #3243
This adds a target *ImageStreamNode into the ImagePipeline. It does this by creating an edge between an
ImageStreamTagNode
and an associatedImageStreamNode
. When the graph is traversed to build theImagePipeline
, the edge is chased and resolved.Output looks like this if the build won't be able to push:
@smarterclayton sidebar: why do we have veneers over the map traversals? "Math is hard"?