Automated dependency update binary #493

chxchx · 2017-07-21T23:57:26Z

@sebastienvas @yutongz

cron jobs and other relevant setup to invoke this binary will be made in another PR

Design Doc

istio-testing · 2017-07-22T00:15:23Z

Jenkins job istio/presubmit passed

sebastienvas · 2017-07-22T23:42:41Z

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> .

sebastienvas

PTAL

sebastienvas · 2017-07-24T16:00:51Z

toolbox/dependency.go

+	repoInfoStore = make(map[string]repoInfo) // maps repo name to its info
+)
+
+func buildDepsGraph() {


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.

sebastienvas · 2017-07-24T16:06:23Z

toolbox/main.go

+	if err != nil {
+		return "", err
+	}
+	return fmt.Sprintf("https://%s:%s@github.com/%s/%s.git", acct, token, acct, repo), nil


The repo is public, do you really need the user and password here ?

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.

sebastienvas · 2017-07-24T16:09:32Z

toolbox/dependency.go

+			url = "https://storage.googleapis.com/istio-build/proxy/envoy-alpha-" + PROXY + ".tar.gz",
+		)
+	*/
+	constant


Can we rename this to something meaningful ?

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.

sebastienvas · 2017-07-24T16:11:25Z

toolbox/main.go

+	if err != nil {
+		return line, err
+	}
+	latestStableCommit, err := util.Shell(


Can t you use the github api to figure this out ?
https://developer.github.com/v3/repos/branches/

Good catch!

sebastienvas · 2017-07-24T16:18:09Z

toolbox/dependency.go

+}
+
+type repoInfo struct {
+	depFile string // where in the *parent* repo such dependecy isrecorded


There could be more than one file. I would move the file up to dependency as there could one file per deps.

Totally Agree.

sebastienvas · 2017-07-24T16:30:32Z

toolbox/main.go

+	for i := 0; i < len(lines); i++ {
+		for _, dep := range rinfo.deps {
+			switch dep.depType {
+			case bazelRule:


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.

That is a smart way to update WORKSPACE. Done.

sebastienvas · 2017-07-24T16:40:52Z

toolbox/main.go

+	if err := os.Chdir(repo); err != nil {
+		return err
+	}
+	branch := fmt.Sprintf("autoUpdateDeps%v", time.Now().UnixNano())


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)

sebastienvas · 2017-07-24T16:42:56Z

toolbox/dependency.go

+}
+
+var (
+	repoInfoStore = make(map[string]repoInfo) // maps repo name to its info


should not it be map[string][]repoInfo. In theory a parent has more than one deps.

This might be no longer relevant as we decided that we should put such dependency metadata into a file for each repo.

ldemailly · 2017-07-25T01:45:27Z

cc @costinm

chxchx · 2017-07-25T03:12:22Z

PTAL

There is only one thing left. For reliable editing of WORKSPACE or repositories.bzl, we hope to implement what I called "dependency hoisting"

Example:

	new_git_repository(
		name = "mixerapi_git",
		commit = "ee9769f5b3304d9e01cd7ed6fb1dbb9b08e96210",
		remote = "https://github.com/istio/api.git",
	)

becomes

	MIXERAPI = "ee9769f5b3304d9e01cd7ed6fb1dbb9b08e96210"
	new_git_repository(
		name = "mixerapi_git",
		commit = MIXERAPI,
		remote = "https://github.com/istio/api.git",
	)

I will refactor getDeps() function once the WORKSPACE and other bazel relevant files in all istio repos have conformed to such standard.

istio-testing · 2017-07-25T03:15:16Z

Jenkins job istio/presubmit passed

sebastienvas · 2017-07-25T03:24:57Z

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> .

chxchx · 2017-07-25T03:26:20Z

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.

istio-testing · 2017-07-25T03:28:19Z

Jenkins job istio/presubmit passed

istio-testing · 2017-07-25T21:56:44Z

Jenkins job istio/presubmit passed

istio-testing · 2017-07-25T22:07:32Z

@chxchx: The following test failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
prow/istio-presubmit.sh	`79e7605`	link	`@istio-testing bazel test this`

Instructions 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.

sebastienvas · 2017-07-26T05:27:57Z

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

googlebot added the cla: yes label Jul 21, 2017

sebastienvas suggested changes Jul 24, 2017

View reviewed changes

Automatic Update Dependency Script

29fb1b5

chxchx force-pushed the depsUpdate branch from 07fcfa0 to b3875f6 Compare July 25, 2017 02:52

Address code review

2b1e461

chxchx force-pushed the depsUpdate branch from b3875f6 to 2b1e461 Compare July 25, 2017 03:06

Handle istio/istio repo dependency update

79e7605

chxchx closed this Jul 27, 2017

chxchx mentioned this pull request Jul 27, 2017

Automated dependency update binary istio/test-infra#336

Merged

guptasu pushed a commit to guptasu/istio that referenced this pull request Jun 11, 2018

Move a couple Mixer docs (istio#493)

0142cbd

howardjohn pushed a commit to howardjohn/istio that referenced this pull request Jan 12, 2020

Use global trust domain (istio#493)

c2c49fd

* Use global trust domain * make gen * Fix stuff

vikaschoudhary16 pushed a commit to vikaschoudhary16/istio that referenced this pull request Aug 12, 2024

Merge pull request istio#493 from tetrateio/port/1.21/private-ip

f45269a

Consider private ip if public ip is not set for NodePort services

Automated dependency update binary #493

Automated dependency update binary #493

Uh oh!

Conversation

chxchx commented Jul 21, 2017

Uh oh!

istio-testing commented Jul 22, 2017

Uh oh!

sebastienvas commented Jul 22, 2017 via email

Uh oh!

sebastienvas left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ldemailly commented Jul 25, 2017

Uh oh!

chxchx commented Jul 25, 2017

Uh oh!

istio-testing commented Jul 25, 2017

Uh oh!

sebastienvas commented Jul 25, 2017 via email

Uh oh!

chxchx commented Jul 25, 2017

Uh oh!

istio-testing commented Jul 25, 2017

Uh oh!

istio-testing commented Jul 25, 2017

Uh oh!

istio-testing commented Jul 25, 2017

Uh oh!

sebastienvas commented Jul 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

sebastienvas commented Jul 26, 2017 •

edited

Loading