Automated dependency update binary by chxchx · Pull Request #336 · istio/test-infra

chxchx · 2017-07-27T00:02:58Z

Release note:

NONE

chxchx · 2017-07-27T00:03:58Z

/assign @sebastienvas

chxchx · 2017-07-27T00:21:18Z

The base of this code change was migrated from PR#493 on istio/istio

Additionally, we assume each repo now hosts a file where its dependency info is recorded (see PR #89 on istio/mixerclient). Hence, the dependency update script now accesses dependency info from such file.

sebastienvas · 2017-07-27T02:18:13Z

toolbox/deps_update/main.go

+	if err != nil {
+		return err
+	}
+	mixerSHA, err := githubClnt.getHeadCommitSHA("mixer", "stable")


What about auth for istio-ca ?

nlandolfi

nice!

most of my comments are a function of

trying to get away from package level variables
trying to be idiomatic
trying to fail the program early if one of assumptions isn't met (like loading token immediately)

just suggestions!

nlandolfi · 2017-07-27T16:23:56Z

toolbox/deps_update/dependency.go

+package main
+
+/*
+	Assumes Dependency Hoisting in all bazel dependency files. Example:


s/Hoisting/Hosting

I actually do want to say hoisting here. It means pulling the commit SHA up and out of the bazel rules, defining is as constant string, and referencing it in the bazel rule.

nlandolfi · 2017-07-27T16:26:33Z

toolbox/deps_update/githubClient.go

@@ -0,0 +1,111 @@
+// Copyright 2017 Istio Authors


I think it might be a go thing to name files snake case github_client.go

but not a big deal i don't think.

or githubclient

I actually see a mix of camel case and snake case in our repos for go files and got confused too. But I guess I will leave it like this unless people have a strong opinion

nlandolfi · 2017-07-27T16:28:04Z

toolbox/deps_update/githubClient.go

+		Body:  &body,
+	}
+	log.Printf("Creating a PR with Title: \"%s\" for repo %s", title, repo)
+	pr, _, err := g.client.PullRequests.Create(context.TODO(), *owner, repo, &req)


i think either parameterize this function by a context or use context.Background() -- not TODO

nlandolfi · 2017-07-27T16:29:35Z

toolbox/deps_update/util.go

+	return hex.EncodeToString(hash[:])
+}
+
+// Shell run command on shell and get back output and error if get one


nit: s/run/runs

haha I took this code from istio/istio. I blame Seb on this.

nlandolfi · 2017-07-27T16:30:23Z

toolbox/deps_update/util.go

+// GetMD5Hash generates an MD5 digest of the given string
+func GetMD5Hash(text string) string {
+	hash := md5.Sum([]byte(text))
+	return hex.EncodeToString(hash[:])


is the [:] making a copy of the hash bytes and do we need to make a copy since you don't do anything with hash?

possibly can delete, you only use helper once, and might be able to be replaced with

hex.EncodeToString(md5.Sum([]byte(text)))

maybe

The [:] operator creates a slice (type []byte) from an array (type [16]byte). (See full explanation here) It is necessary to make such conversion here since md5.Sum() returns [16]byte but hex.EncodeToString wants []byte.

I think this function is generically useful to not just this particular feature/script but also the rest of test-infra so putting it under util is in my opinion a good idea.

nlandolfi · 2017-07-27T16:46:39Z

toolbox/deps_update/util.go

+}
+
+// Shell run command on shell and get back output and error if get one
+func Shell(format string, args ...interface{}) (string, error) {


my suggestion here is maybe don't bother with writing a helper unless you need the extra logging

rather than doing the fmt.Sprintf with the formatting you can just use the underlying signature and pass in the executable and list of args right? seems funky that you always create the string only to split it again...

like exec.Command("./some/script.sh", []string{"-p", "flag", "-d", fmt.Sprintf("%d", integerSomething)}).CombinedOutput() seems nearly equivalent

nlandolfi · 2017-07-27T16:49:13Z

toolbox/deps_update/main.go

+
+var (
+	repo       = flag.String("repo", "", "Update dependencies of only this repository")
+	githubClnt *githubClient


doesn't seem like this should be a global package level variable

It is declared on a global scope so that other functions in main.go could easily access githubClient functions. The intention is not providing package level visibility. Any way we could do better?

shoot just saw this -- gotcha, the other option is to pass it down as arguments to functions.

nlandolfi · 2017-07-27T16:50:39Z

toolbox/deps_update/main.go

+// Update the commit SHA reference in a given line from dependency file
+// to the latest stable version
+// returns the updated line
+func replaceCommit(line string, dep dependency) (string, error) {


this should probably either take as argument a pointer to a client or have a client reciever

i.e., replaceCommit(gc *githubClient, line string, dep dependency) or func (gc *githubClient) replaceCommit(line string, dep dependency)

Any reason why such indirection is necessary?

nlandolfi · 2017-07-27T16:51:26Z

toolbox/deps_update/main.go

+}
+
+// Get the list of dependencies of a repo
+// Assumes repo has been cloned to local


cloned to local? or cloned locally? does this program assume that we are in root directory of the repo?

The assumption has been removed

nlandolfi · 2017-07-27T16:51:53Z

toolbox/deps_update/main.go

+
+// Generates an MD5 digest of the version set of the repo dependencies
+// useful in avoiding making duplicate branches of the same code change
+func fingerPrint(deps []dependency) (string, error) {


i think also should either take a github client or be a method with a gc receiver.

nlandolfi · 2017-07-29T00:59:58Z

ohhhhh gotcha -- SGTM

…

On Fri, Jul 28, 2017 at 5:56 PM Charles Xu ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In toolbox/deps_update/dependency.go <#336 (comment)>: > +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package main + +/* + Assumes Dependency Hoisting in all bazel dependency files. Example: I actually do want to say hoisting here. It means pulling the commit SHA up and out of the bazel rules, defining is as constant string, and referencing it in the bazel rule. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#336 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ACBRUVxgdZ8NGN8BDtyKqsSqoN_nFeXLks5sSoNEgaJpZM4Okose> .

nlandolfi · 2017-07-29T01:01:48Z

ya seb pointed out to me that mixer does camel case

…

On Fri, Jul 28, 2017 at 6:00 PM Charles Xu ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In toolbox/deps_update/githubClient.go <#336 (comment)>: > @@ -0,0 +1,111 @@ +// Copyright 2017 Istio Authors I actually see a mix of camel case and snake case in our repos for go files and got confused too. But I guess I will leave it like this unless people have a strong opinion — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#336 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ACBRUexz6UpVk0216FgorHJNfWmbSPvvks5sSoQpgaJpZM4Okose> .

chxchx · 2017-07-30T22:16:12Z

PTAL

Comments above have been addressed. Specifically, failed PRs are now closed in subsequent runs, istio-ca is taken care of, code style has improved, and duplicate code with github_helper has been refactored. An example PR created by this script is here. This update script has to work with istio.deps file at root, and please also take a look at istio/istio#510

yutongz · 2017-07-31T03:54:33Z

toolbox/deps_update/main.go

+// Returns the updated line
+func replaceCommit(line string, dep dependency) (string, error) {
+	idx := strings.Index(line, "\"")
+	return line[:idx] + "\"" + dep.LastStableSHA + "\",", nil


No need to have an error return if the function never returns an error.

You are absolutely right. There has been a refactor and I forgot to take the error out.

yutongz · 2017-07-31T04:03:19Z

toolbox/deps_update/main.go

+	caHub := "docker.io/istio"
+	istioctlURL := fmt.Sprintf(
+		"https://storage.googleapis.com/istio-artifacts/pilot/%s/artifacts/istioctl", pilotSHA)
+	hub := "gcr.io/istio-testing"


Please make something like docker hub, gcs storage configurable from flags or read from somewhere, try not hardcoded.

I agree. It is now read from istio.VERSION directly.

yutongz · 2017-07-31T04:25:44Z

toolbox/deps_update/main.go

+	caHub := "docker.io/istio"
+	istioctlURL := fmt.Sprintf(
+		"https://storage.googleapis.com/istio-artifacts/pilot/%s/artifacts/istioctl", pilotSHA)
+	hub := "gcr.io/istio-testing"


Try not to hardcoded something like docker hubs and gcs storage, make it configurable and read it from flags or somewhere else.

yutongz · 2017-07-31T04:25:50Z

toolbox/deps_update/main.go

+// Returns the updated line
+func replaceCommit(line string, dep dependency) (string, error) {
+	idx := strings.Index(line, "\"")
+	return line[:idx] + "\"" + dep.LastStableSHA + "\",", nil


If no error is going to be returned, then remove error from return values.

yutongz · 2017-07-31T04:26:36Z

toolbox/deps_update/main.go

+	if _, err := util.Shell("git add *"); err != nil {
+		return err
+	}
+	if _, err := util.Shell("git commit -m Update_Dependencies"); err != nil {


Maybe you can use "git commit -am "

yutongz · 2017-07-31T04:30:23Z

toolbox/deps_update/main.go

+		}
+		for _, r := range repos {
+			if err := updateDependenciesOf(r); err != nil {
+				log.Panicf("Failed to udpate dependency: %v\n", err)


Careful using panic, you may still want to update other repos even the first one hit an error

yutongz · 2017-07-31T04:35:01Z

toolbox/deps_update/main.go

+	return nil
+}
+
+func main() {


I suppose it is a periodic job, then why not use goroutine and wake up every midnight

I thought the intention is to set up the cron jobs thru jenkins/prow and this binary does not care when it is invoked (ordering of repo) or how it is triggered (cron versus PR). I was trying to leave for room for the future if we want to update istio project by building from leaves.

chxchx · 2017-07-31T21:47:59Z

PTAL
An example PR on istio/istio created by this script can be found here https://github.com/chxchx/istio/pull/9/files

sebastienvas · 2017-08-01T16:55:45Z

you should use pretty print on the json file. That being said, this looks promising

sebastienvas · 2017-07-31T16:45:58Z

toolbox/deps_update/main.go

+// useful in avoiding making duplicate branches of the same code change
+// Also updates dependency objects deserialized from istio.deps
+func fingerPrintAndUpdateDepSHA(repo string, deps *[]dependency) (string, error) {
+	digest, err := githubClnt.getHeadCommitSHA(repo, "master")


We want this to work also for release branches, so you should make the branch as a flag.

sebastienvas · 2017-07-31T16:46:59Z

toolbox/deps_update/githubClient.go

+func (g githubClient) closeFailedPullRequests(repo string) error {
+	log.Printf("Search for failed auto PRs to update dependencies in repo %s", repo)
+	options := github.PullRequestListOptions{
+		Base:  "master",


Same comment here. Use the branch as a flag.

sebastienvas · 2017-07-31T16:48:54Z

toolbox/deps_update/main.go

+// Assumes at the root of istio directory
+// Runs the updateVersion.sh script
+func updateIstioDeps() error {
+	pilotSHA, err := githubClnt.getHeadCommitSHA("pilot", "stable")


Do you have to hard code this? Can you look at the istio.deps files and generate this from the json files?

There has to be some hadcoding in order to use install/updateVersion.sh because each dependency is passed in with different flag and hub. Such flags and hubs are only applicable to istio/istio so they should not be put into the json file. I did refactor this such that SHAs are read from deps array (read from json file) but I still have to specify the name such as "pilot" and "mixer" to properly use the flags. Let me know if you have smarter ways to do this.

sebastienvas · 2017-07-31T16:54:12Z

toolbox/deps_update/main.go

+	istioctlURL := fmt.Sprintf(
+		"https://storage.googleapis.com/istio-artifacts/pilot/%s/artifacts/istioctl", pilotSHA)
+	hub := "gcr.io/istio-testing"
+	cmd := fmt.Sprintf("./install/updateVersion.sh -p %s,%s -x %s,%s -i %s -c %s,%s",


Maybe dynamically update the flags based which deps needs to be updated.

It is hard since there is also correspondence between dependency and hub, and istioctlURL does not have a hub. The updateVersion.sh is just a bit ad hoc so I think what I have here is actually the most concise

sebastienvas · 2017-07-31T17:54:24Z

toolbox/deps_update/githubClient.go

+	}
+	title := prTitlePrefix + repo
+	body := "This PR will be merged automatically once checks are successful."
+	base := "master"


Same comment here. Make the branch a flag.

sebastienvas · 2017-07-31T18:02:49Z

toolbox/deps_update/githubClient.go

+	}
+	var multiErr error
+	for _, pr := range prs {
+		if strings.Contains(*pr.Title, prTitlePrefix) {


Don't you want to look only those that have the prefix ?

chxchx · 2017-08-03T01:36:43Z

PTAL

Minor dispute over the map thing but I agree with everything else. An Example PR created by the script is here https://github.com/chxchx/istio/pull/11/files

sebastienvas · 2017-08-03T02:30:58Z

The PR that you pointed is still using gcr.io/istio-io in some of those update.

sebastienvas

I can t see your comments. Let s do an hour of pair programming tomorrow so that I can understand where you are going

sebastienvas · 2017-08-03T02:38:40Z

toolbox/deps_update/main.go

+	owner      = flag.String("owner", "istio", "Github Owner or org")
+	tokenFile  = flag.String("token_file", "", "File containing Github API Access Token")
+	baseBranch = flag.String("base_branch", "master", "Branch from which the deps update commit is based")
+	caHub      = flag.String("ca_hub", "", "Optional. Where new CA image is hosted")


Just create one hub for all of them. It is the same we use for testing.

Are you sure? Because on istio master, the three hubs in istio.VERSION are all different. Unless you are saying they all resolve to the same host but maybe having the option of updaing only one of the hubs and not others could be useful.

chxchx · 2017-08-03T03:09:07Z

You are absolutely right. I forgot to rebase from istio/istio. Please take a look on this https://github.com/chxchx/istio/pull/12/files

Also, this PR now contingent on the correction I just made at istio/istio#534 so please also take a look on that.

chxchx · 2017-08-07T19:58:50Z

PTAL

I hope I understand what @sebastienvas wants for the hub flag. Example PR created by this script can be found here https://github.com/chxchx/istio/pull/14/files

sebastienvas · 2017-08-10T19:56:43Z

/lgtm

istio-merge-robot · 2017-08-10T19:56:59Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: chxchx, sebastienvas, yutongz

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these OWNERS Files:

~~OWNERS~~ [sebastienvas,yutongz]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

istio-merge-robot · 2017-08-10T19:57:11Z

/test all [submit-queue is verifying that this PR is safe to merge]

istio-merge-robot · 2017-08-10T19:59:15Z

Automatic merge from submit-queue

Automatic merge from submit-queue Automated dependency update binary **Release note**: ```release-note NONE ```

…af61ae (istio#336)

Dependency Update Binary

7ae80b6

googlebot added the cla: yes Set by the Google CLA bot to indicate the author of a PR has signed the Google CLA. label Jul 27, 2017

istio-merge-robot added the release-note-label-needed label Jul 27, 2017

Get dependencies from each repo

524ab11

chxchx force-pushed the depsUpdate branch from 61de0d4 to 524ab11 Compare July 27, 2017 00:11

istio-merge-robot added release-note-none and removed release-note-label-needed labels Jul 27, 2017

sebastienvas self-requested a review July 27, 2017 02:13

sebastienvas reviewed Jul 27, 2017

View reviewed changes

nlandolfi reviewed Jul 27, 2017

View reviewed changes

WIP: Extract common git utils and check failed PR

6a26ba7

Address code review

e2d67d8

chxchx force-pushed the depsUpdate branch from bee8029 to d2e1f14 Compare July 29, 2017 22:28

Close failed PRs and their branches

5fffa7e

chxchx force-pushed the depsUpdate branch from d2e1f14 to 5fffa7e Compare July 29, 2017 22:38

chxchx mentioned this pull request Jul 30, 2017

Initial import of istio.deps istio/istio#510

Merged

Update Deps on istio

4bc6161

chxchx force-pushed the depsUpdate branch from 5f411a4 to 4bc6161 Compare July 30, 2017 22:10

yutongz reviewed Jul 31, 2017

View reviewed changes

Avoid hard coding docker hubs and gcs storage

b11c424

sebastienvas suggested changes Aug 1, 2017

View reviewed changes

Add base_branch flag

cd6b127

chxchx force-pushed the depsUpdate branch from ab1f4d2 to cd6b127 Compare August 1, 2017 20:02

chxchx force-pushed the depsUpdate branch from e5c1e64 to b182fa2 Compare August 3, 2017 01:11

Take hubs as flags

8d74484

chxchx force-pushed the depsUpdate branch from b182fa2 to 8d74484 Compare August 3, 2017 01:14

sebastienvas suggested changes Aug 3, 2017

View reviewed changes

Comprehensive digest

9dfc9f6

chxchx force-pushed the depsUpdate branch 2 times, most recently from 61d2707 to 7acfe42 Compare August 3, 2017 05:05

Remove all flags on updateVersion.sh

6217e70

chxchx force-pushed the depsUpdate branch 2 times, most recently from 755da31 to 81bf23c Compare August 7, 2017 19:54

Going back to using updateVersion script

32eff61

chxchx force-pushed the depsUpdate branch from 81bf23c to 32eff61 Compare August 7, 2017 19:57

istio-merge-robot added the approved label Aug 9, 2017

yutongz removed the approved label Aug 9, 2017

istio-merge-robot added the approved label Aug 9, 2017

sebastienvas approved these changes Aug 10, 2017

View reviewed changes

istio-testing assigned sebastienvas Aug 10, 2017

istio-testing added the lgtm label Aug 10, 2017

istio-merge-robot merged commit 6ddc565 into istio:master Aug 10, 2017

chxchx deleted the depsUpdate branch August 10, 2017 20:11

yutongz pushed a commit to yutongz/test-infra that referenced this pull request Aug 16, 2017

Automated dependency update binary (istio#336)

ce96adb

Automatic merge from submit-queue Automated dependency update binary **Release note**: ```release-note NONE ```

soloio-bot pushed a commit to soloio-bot/test-infra that referenced this pull request Jun 22, 2023

Update build-tools to release-1.16-914b2b708396e4521304b241116098a0ff…

af8d793

…af61ae (istio#336)

Conversation

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

Uh oh!

Uh oh!

chxchx commented Jul 27, 2017

Uh oh!

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

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nlandolfi 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!

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!

nlandolfi commented Jul 29, 2017 via email

Uh oh!

nlandolfi commented Jul 29, 2017 via email

Uh oh!

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

Uh oh!

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

chxchx commented Jul 27, 2017 •

edited

Loading

chxchx commented Jul 27, 2017 •

edited

Loading

chxchx commented Jul 30, 2017 •

edited

Loading

chxchx Jul 31, 2017 •

edited

Loading

sebastienvas commented Aug 1, 2017 •

edited

Loading