-
Notifications
You must be signed in to change notification settings - Fork 4.8k
Output build dependencies of a specific image repository #1229
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
Conversation
|
Also how should I name this utility? |
|
it should be a command under the experimental command set... "build-tree" or "build-chain" would be my first name ideas. btw i don't know that we need an issue and a trello card for this, we can discuss it in the card. as for the specific format, let's get Jay to weigh in. i'll send a note. |
WIth some luck I'll turn this into a pull request. |
|
We should have three formats: internally, in memory, a structure that represents the data the first two should be very closely coupled - they don't have to be formal API objects, but it would be better to structure them that way so that in the future we can version them. I would think of the graphviz as a special "-o" argument that visualizes them (-o viz) ----- Original Message -----
|
|
I have created the basic structure of the code (data structures, flow, etc). I haven't thoroughly tested it though except starting two builds from the same configuration file just to see if it can find the pushed images (and it does). For testing all of the code, I have to create multiple Dockerfiles that depend on each other, right? :soundsboring: :) |
|
@Kargakis Well this is an interesting question... do we want to illustrate
I think the (1) is sufficient, and given that, you don't need do anything w/ actual dockerfiles..you can just create a set of BuildConfigs that have some ImageChangeTrigger relationship. It doesn't really matter if the actual BuildConfig consumes the new/changed image or not, just that the upstream image changing causes that BuildConfig to run (which in turn produces some new image that may or may not actually be based on the upstream image) |
I don't understand this. How can a build be triggered by an upstream image if that image is not a part of the build chain (ie. one of the images that can be found in FROM of one of the Dockerfiles higher in the tree) ? |
|
@Kargakis nothing prevents me from setting up an ImageChangeTrigger on a build in which the ICT watches image "foo" for changes but the BuildConfig itself does not consume "foo" as part of the build process. |
|
@Kargakis similarly you could setup a Github webhook that triggered your build on pushes to repo Foo even though your build was building repo Bar |
|
@bparees Ok, so to make this clear I am going to check for triggers in configuration files as well as images that explicitly depend on other images (which I already do), right? |
|
@Kargakis no, just triggers. @smarterclayton @csrwng please confirm. |
|
Until deciding how are we going to look for dependencies (see Jay's comment on Trello), I've started writting tests for parts of the code that are independent [of the way we inspect for dependencies] . Most important being the building of the tree structure ( |
|
When running Is anybody familair with it? Tests should pass. Here is a slightly edited version of the algorithm (and tests) using strings instead of api.Images: http://play.golang.org/p/qEo_nGt5Ko |
|
I cleared this up with jay - the approach Ben relayed is the one we are doing for now.
|
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.
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.
The image repositories will exist. Only the images/tags won't.
|
Are we addressing circular dependencies in buildConfigs atm? |
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.
Shouldn't you just take the first argument as the image repo name? (line 45 implies that you do)
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.
There should be a mode for this command that is --all that iterates over all image repositories in the system and creates a graph with multiple roots.
----- Original Message -----
+Output build dependencies of a specific image repository.
+
+Examples:
+
- $ openshift ex build-chain [image-repository]
+`
+func NewCmdBuildChain(f *clientcmd.Factory, parentName, name string)
*cobra.Command {
- cmd := &cobra.Command{
Use: fmt.Sprintf("%s [image-repository]", name),Short: "Output build dependencies of a specific image repository",Long: longDescription,Run: func(cmd *cobra.Command, args []string) {imageRepoName := cmdutil.GetFlagString(cmd, "imagerepo")if imageRepoName == "" { --imagerepo (-i) flag")usageError(cmd, "Must pass an image repository name with theShouldn't you just take the first argument as the image repo name? (line 45
implies that you do)
Reply to this email directly or view it on GitHub:
https://github.com/openshift/origin/pull/1229/files#r26130209
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.
Good catch, I'll just use the first argument.
|
@Kargakis no, we are not. |
|
JSON is in, next thing tbd DOT |
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 believe it's valid for Output.To to specify a namespace (Output.To.Namespace) so this should take that into account. (if not specified it defaults to this namespace)
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.
@bparees the same goes for the DockerImageReference, right?
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.
no, DockerImageReference is a docker pull spec for a registry, it has no association to an openshift namespace.
|
I think we need to specify a tag when running this, because a particular ICT isn't triggered just by an ImageRepo changing, the specific Tag that the ICT watches needs to change. |
The idea is that the user specifies an image repository and gets back a dependency tree. Now, the tags you are talking about can be found in the tree, ie. each node has alongside its name and namespace, the tags by which it depends on another node. |
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.
again needs to include namespace in de-duplication checking.
|
I fixed the labeling plus since we want to distinguish between namespaces I have added them in the name of a node (now called fullName). I don't output namespaces in the graph though because we cannot distinguish the name from the namespace. [1] [1] The DOT parser eventually doesn't accept any special character except underscores. So imagine a node id like : ps. the label of an edge is really the name of a buildConfig |
|
Namespace will be important to most users
|
|
@smarterclayton sure, take a look below where I output namespaces in smaller font |
|
Looks good ----- Original Message -----
|
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.
Is defaultNamespace the right namespace to assign here or should I check first if cfg.Namespace is defined, if so, assign that, if not, assign defaultNamespace ? @bparees
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.
Nvm, got my answer on IRC
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.
Is ref.Name the name of the image or the image repository? @bparees
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.
Nvm v.2, it's the image repo name
|
I think this is ready for one more (hopefully the last) review:) |
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.
it isn't really invalid, it's just an image repo that doesn't trigger any builds, right?
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.
Well it may be that or it may be invalid
|
lgtm and it's experimental so we can iterate a little if we need to. great work @Kargakis let me know if you want it merged, wasn't sure if you had any final tweaks or that's just doc updates. |
@bparees I want to add tags on node names for the DOT output and was thinking about adding an extra flag where we would output all the trees generated by a specific repository which we currently don't support and it's probably something that the RCM guys will probably want. eg. generates all trees from all tags in this repo. Thinking about it, we support outputing all trees but for all image repos and not for one specific. |
|
@karkgakis sounds reasonable. Maybe "--all-tags" |
|
@bparees have a look in the latest commits and I'll squash if everything is fine. I won't add tags next to names for the time being because it gets complicated with children with multiple tags. |
|
I have injected tags into root names (only) because I feel it's better to show root tags than not |
|
@Kargakis still lgtm. |
Track build dependencies via build configurations and output them in json, dot, or ast notation.
|
@bparees cool, you can merge it now |
|
[merge] |
|
continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_requests_openshift3/1259/) (Image: devenv-fedora_1107) |
|
Evaluated for origin up to a2212c3 |
Merged by openshift-bot
|
Changes Unknown when pulling a2212c3 on kargakis:output-dep into * on openshift:master*. |
|
Changes Unknown when pulling a2212c3 on kargakis:output-dep into * on openshift:master*. |
[3.8][backport] Improve patching ovs flow rules in UpdateEgressNetworkPolicyRules
Supported output formats: json, dot, and ast.
DOT output visualized
http://i.imgur.com/LVse6hX.png