Conversation
|
Jenkins job istio/presubmit passed |
|
You might want to rebase your change. There are commits from your previous
PR.
…On Jul 21, 2017 5:15 PM, "istio-bot" ***@***.***> wrote:
Jenkins job istio/presubmit passed
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#493 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AQ80Z18VsWflRIm5zgFq1Tb1FsWUQ5_dks5sQT8egaJpZM4OgADA>
.
|
toolbox/dependency.go
Outdated
There was a problem hiding this comment.
If you use a file in the repo like stated in the dd, you don't need to hard code those. You can directly get the dependencies from the repos.
toolbox/main.go
Outdated
There was a problem hiding this comment.
The repo is public, do you really need the user and password here ?
There was a problem hiding this comment.
We discussed this in person but I shall repeat myself for the record of other contributors. Although cloning a git repo to local requires no authentication, pushing a new branch to the remote does.
toolbox/dependency.go
Outdated
There was a problem hiding this comment.
Can we rename this to something meaningful ?
There was a problem hiding this comment.
As we discussed, we may treat all dependency entries as a constant whose value is a string representing a commit SHA. Doing so also makes WORKSPACE editing much easier. I have made such change in my latest commit.
toolbox/main.go
Outdated
There was a problem hiding this comment.
Can t you use the github api to figure this out ?
https://developer.github.com/v3/repos/branches/
toolbox/dependency.go
Outdated
There was a problem hiding this comment.
There could be more than one file. I would move the file up to dependency as there could one file per deps.
toolbox/main.go
Outdated
There was a problem hiding this comment.
This seems flaky as the order might change. You can actually change the file such that it is done as a constant. This is why all the other constant are there.
There was a problem hiding this comment.
That is a smart way to update WORKSPACE. Done.
toolbox/main.go
Outdated
There was a problem hiding this comment.
That s unique, but that does not prevent you from creating twice the same PR. if should be a function of (Parent Repo, Deps Repos, Branch Name, Head of Branch, SHAs to update)
toolbox/dependency.go
Outdated
There was a problem hiding this comment.
should not it be map[string][]repoInfo. In theory a parent has more than one deps.
There was a problem hiding this comment.
This might be no longer relevant as we decided that we should put such dependency metadata into a file for each repo.
|
cc @costinm |
|
PTAL There is only one thing left. For reliable editing of WORKSPACE or repositories.bzl, we hope to implement what I called "dependency hoisting" I will refactor getDeps() function once the WORKSPACE and other bazel relevant files in all istio repos have conformed to such standard. |
|
Jenkins job istio/presubmit passed |
|
Try to think of way to apply this to istio repo.
On Jul 24, 2017 8:15 PM, "istio-bot" <notifications@github.com> wrote:
Jenkins job istio/presubmit passed
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#493 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AQ80Z8QAuHZbeP1an1QO8e2dSbxsRgtpks5sRV3HgaJpZM4OgADA>
.
|
|
I have noticed the work @costinm has been doing, which is also an effort to address the evolving dependencies among the istio repos. I like his idea a lot, but before the discussion settles and given the p0 demand for this feature, maybe I should finish this (least intrusive to current architecture) to set in motion the automation of dependency update and together searching and implementing a cleaner solution right after. But do let me know if I should channel my effort elsewhere. |
|
Jenkins job istio/presubmit passed |
|
Jenkins job istio/presubmit passed |
|
@chxchx: The following test failed, say
DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
|
I actually noticed that you created this form istio/istio. It should be in istio/test-infra. could you move it there in toolbox as well ? Also, after talking to @yutongz, I realized that there is currently no easy way to extract metrics repos anymore to know how far behing they are. This will be important for the build cop, as they need to know which repos as outdated deps. Start thinking about it such that we can do that in a new PR. |
* Use global trust domain * make gen * Fix stuff
Consider private ip if public ip is not set for NodePort services
@sebastienvas @yutongz
cron jobs and other relevant setup to invoke this binary will be made in another PR
Design Doc