Add vendor-check script, Makefile target and CI step #581

azer · 2018-01-30T10:00:00Z

It adds vendor-check target, copying the validation script in dep repository.

Closes #556

mandrigin

Looks good! A few changes are necessary.

Also, it would be nice to validate that the script actually returns a non-zero answer if contents of vendor are changed. Now it shows warning: no common commits because it checks against the wrong repo.

mandrigin · 2018-01-30T10:55:36Z

.validate-vendor.sh

+			echo
+		} >&2
+		false
+  else


Seems like indentation went funky in this file, let's fix that.

mandrigin · 2018-01-30T10:57:59Z

.validate-vendor.sh

+set -e -o pipefail
+
+if [ -z "$VALIDATE_UPSTREAM" ]; then
+	VALIDATE_REPO='https://github.com/golang/dep.git'


It should be the status-go repo, right?

mandrigin · 2018-01-30T10:58:13Z

.validate-vendor.sh

+
+if [ -z "$VALIDATE_UPSTREAM" ]; then
+	VALIDATE_REPO='https://github.com/golang/dep.git'
+	VALIDATE_BRANCH='master'


We use develop as a target branch.

mandrigin · 2018-01-30T11:42:24Z

@azer looks good now!
One last point. It fails on Travis now due to dep not being installed. Let's add dep-install as a Makefile target and add it to the install: phase of .travis.yml.

azer · 2018-01-31T04:54:09Z

@mandrigin Thanks for the feedback, I've made the fixes requested, also tested it on a branch with additions into vendor/ folder. Here is what the failure exit looks like;

The contents of vendor differ after "dep ensure":

 D vendor/github.com/azer/logger/README.md
 D vendor/github.com/azer/logger/examples/attrs.go
 D vendor/github.com/azer/logger/examples/multiple.go
 D vendor/github.com/azer/logger/examples/programmatical.go
 D vendor/github.com/azer/logger/examples/simple.go
 D vendor/github.com/azer/logger/examples/timer-performance.go
 D vendor/github.com/azer/logger/format.go
 D vendor/github.com/azer/logger/log.go
 D vendor/github.com/azer/logger/logger.go
 D vendor/github.com/azer/logger/runtime.go
 D vendor/github.com/azer/logger/standard-output.go

Make sure these commands have been run before committing.

mandrigin · 2018-01-31T13:31:34Z

@azer Looks good! I'll ask one peer of mine to review it too.

themue

Only one question, rest LGTM.

themue · 2018-01-31T13:40:07Z

ci/validate-vendor.sh

+# for our repo and makes sure that it was done in a valid way.
+#
+# This file is a slightly modified copy of https://github.com/golang/dep/blob/master/hack/validate-vendor.bash
+# The only change made was checking if `dep` was available and installing it if necessary.


Isn't this check done with dep-install in Makefile?

Yup, just deleted it

adambabik · 2018-01-31T14:25:33Z

ci/validate-vendor.sh

+
+if [ -z "$VALIDATE_UPSTREAM" ]; then
+	VALIDATE_REPO='https://github.com/status-im/status-go'
+	VALIDATE_BRANCH='develop'


Shouldn't branch be taken from env and only default to develop? We merge almost every branch to develop but it should change soon.

adambabik · 2018-01-31T14:26:09Z

ci/validate-vendor.sh

+fi
+
+IFS=$'\n'
+files=( $(validate_diff --diff-filter=ACMR --name-only -- 'Gopkg.toml' 'Gopkg.lock' 'vendor/' || true) )


If the if statement above won't be executed, validate_diff won't be defined, right?

Yes, if statement will always be executed unless we define VALIDATE_UPSTREAM before calling this script.

adambabik · 2018-01-31T14:28:08Z

ci/validate-vendor.sh

+
+set -e -o pipefail
+
+if [ -z "$VALIDATE_UPSTREAM" ]; then


I hate bash for their super hard to remember expression. Can you please add a comment // if $VALIDATE_UPSTREAM is null or empty string do bla bla bla...?

Yes, I've added some more comments

Hmm, do we even need this option?

adambabik · 2018-01-31T14:29:30Z

ci/validate-vendor.sh

+files=( $(validate_diff --diff-filter=ACMR --name-only -- 'Gopkg.toml' 'Gopkg.lock' 'vendor/' || true) )
+unset IFS
+
+if [ ${#files[@]} -gt 0 ]; then


I have no idea what ${#files[@]} does (getting a number of files?). Can you please add a comment?

Splits the content of files variable by whitespaces and gets the number of the lines. Just added a comment for it.

adambabik · 2018-01-31T14:35:35Z

ci/validate-vendor.sh

+			echo
+		} >&2
+		false
+  else


Formatting? Shouldn't this else be aligned with if [ "$diffs" ]; then?

adambabik · 2018-01-31T14:38:10Z

I know it's copied from dep repo but it would be great to add a general comment what this bash script actually does in 2-3 sentences.

azer · 2018-02-01T04:06:23Z

@adambabik defaulted branch to "develop", added some comments & fixed tabbing 👍

adambabik

Thanks @azer. LGTM

mandrigin · 2018-02-01T17:15:11Z

@azer thanks for your contribution 🎉

E.Azer Koçulu added 2 commits January 30, 2018 18:42

Add vendor-check script, Makefile target and CI step

d476a0c

Add notes about the changes made in validation script

8a5f373

mandrigin suggested changes Jan 30, 2018

View reviewed changes

E.Azer Koçulu added 3 commits January 30, 2018 19:00

Move validation script to ci folder, as suggested in another PR

d1d2244

Fix repo URI & branch

9d9dce1

Fix tabbing

5fc0185

Add dep install step to travis config

c96b707

mandrigin approved these changes Jan 31, 2018

View reviewed changes

themue approved these changes Jan 31, 2018

View reviewed changes

adambabik suggested changes Jan 31, 2018

View reviewed changes

Add some comments to explain bash conditions

4bede3b

Read branch from the environment, default it to "develop"

e9c835f

adambabik approved these changes Feb 1, 2018

View reviewed changes

mandrigin merged commit 8b56060 into status-im:develop Feb 1, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add vendor-check script, Makefile target and CI step #581

Add vendor-check script, Makefile target and CI step #581

azer commented Jan 30, 2018 •

edited

Loading

mandrigin left a comment

mandrigin Jan 30, 2018

mandrigin Jan 30, 2018

mandrigin Jan 30, 2018

mandrigin commented Jan 30, 2018

azer commented Jan 31, 2018 •

edited

Loading

mandrigin commented Jan 31, 2018

themue left a comment

themue Jan 31, 2018

azer Feb 1, 2018

adambabik Jan 31, 2018

azer Feb 1, 2018

adambabik Jan 31, 2018

azer Feb 1, 2018 •

edited

Loading

adambabik Jan 31, 2018

azer Feb 1, 2018

mandrigin Feb 1, 2018

adambabik Jan 31, 2018

azer Feb 1, 2018

adambabik Jan 31, 2018

azer Feb 1, 2018

adambabik commented Jan 31, 2018

azer commented Feb 1, 2018 •

edited

Loading

adambabik left a comment

mandrigin commented Feb 1, 2018

Add vendor-check script, Makefile target and CI step #581

Add vendor-check script, Makefile target and CI step #581

Conversation

azer commented Jan 30, 2018 • edited Loading

mandrigin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mandrigin commented Jan 30, 2018

azer commented Jan 31, 2018 • edited Loading

mandrigin commented Jan 31, 2018

themue left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

azer Feb 1, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adambabik commented Jan 31, 2018

azer commented Feb 1, 2018 • edited Loading

adambabik left a comment

Choose a reason for hiding this comment

mandrigin commented Feb 1, 2018

azer commented Jan 30, 2018 •

edited

Loading

azer commented Jan 31, 2018 •

edited

Loading

azer Feb 1, 2018 •

edited

Loading

azer commented Feb 1, 2018 •

edited

Loading