CI/build adjustments by costinm · Pull Request #3599 · istio/istio

costinm · 2018-02-18T19:36:12Z

pull master
go 1.10 was released
pilot tests capturing junit-style test result (thanks @nmittler for converting to std tests)
pilot test also using the recommended release style ( istio-system ) and fixed ns so it's easy
to re-run and test after (only used in circleci, new target). The new target also works manually.
adjusted the pilot labels to allow upgrade tests ( will install istio.yaml first ). Currently pilot
tests are not compatible with normal istio configs.

ldemailly

thx for tackling this, see below:

ldemailly · 2018-02-18T19:40:33Z

          sudo chown -R circleci /go
          mkdir -p /home/circleci/logs
      - checkout
+      - run: make git/master


please don't put / in a (.phony) make target

how is it different from "make pull" that's already there ? try that one ? (you can remove submodule-sync if you do make pull)

in which case/why would the just checked out source tree need a pull ?

Re. "/" - why not ? OpenWRT uses this convention (package/foo), I don't like inventing new delimiters and the targets need to be organized a bit eventually. Replaced it with a . since we use it in few targets to avoid another bikesheding, but like / more.

Added comments - it's what you asked for, pulling the changes from master. Submodule-sync seems to sync vendor - not sure what the git pull does there, is redundant with checkout in CI (as you mentioned, we just checked out), but I assumed it's for dev using make ? I didn't put it there, the new one is pulling from master into the branch.

x/y is a path for instance you would make bin/foo if you're target is not a real target/file, don't use a / it's just confusing. thanks for the change

yes the make pull I added for dev workflow to replace git pull and take care of the submodule part, so you're saying this new target is for CI to merge master on the PR?
maybe should be --ff-only ? somehow prow uses 'merge' but I don't know/whichever works

will it abort the build in a way that's easy to understand if the PR has conflicts ?

ldemailly · 2018-02-18T19:41:56Z


+# Pull from git master. To be used in CI or by developers, assumes the
+# remote is called 'origin' (git default). Will fail on conflicts
+git/master:


try/modify to make it work the existing 'pull' target line 180

ldemailly · 2018-02-18T19:42:36Z

@@ -1,13 +1,13 @@
 # DO NOT EDIT THIS FILE MANUALLY instead use


revert this (maybe we can put it in gitignore or some way to not change the one from source tree when we use it)

…uild0218

ldemailly · 2018-02-19T16:52:47Z

      - checkout
      - run: make submodule-sync
      - run: make init
+      - run: git.pullmaster


you have it above the submodule-sync in the other build above; it probably should be first in case there is a vendor change in master

ldemailly · 2018-02-19T16:55:43Z

-       if curl --version | grep Protocols  | grep https; then
-	   DOWNLOAD_COMMAND='curl -Lo -'
+       if curl --version | grep Protocols  | grep https > /dev/null; then
+	   DOWNLOAD_COMMAND='curl -Lso -'


add -f while at it (#3231)

I'll leave it for a specific fix, not sure how it interact with the wget ( or why we even have wget ).

Also there is a tar extraction, cleanup - seems better as a separate PR.

if you're changing the curl command options please add f to curl options / why not?

I'm changing the curl command to make it less verbose.
I don't think -f should be used, and even if it is - I don't think it's reasonable to ask to add unrelated features/fixes to a PR.

( -f would need to be tested, logic may need to be changed, there is also the wget stuff around - I don't want to add that complexity to this PR, and I don't think it's a P0)

ldemailly · 2018-02-19T16:56:46Z

-    ${DOWNLOAD_COMMAND} https://storage.googleapis.com/istio-build/proxy/envoy-$PROXY.tar.gz | tar xz
-    cp usr/local/bin/envoy $ISTIO_GO/vendor/envoy-$PROXYVERSION
+    time ${DOWNLOAD_COMMAND} https://storage.googleapis.com/istio-build/proxy/envoy-$PROXY.tar.gz | tar xz
+    echo "Downloading envoy $PROXY using $DOWNLOAD_COMMAND"


please put the echo before the command not after so we know what it's doing/how it fails (#3231)

ldemailly · 2018-02-19T16:57:39Z

+         --ns istio-system \
+          --logtostderr \
+        -n istio-test \
+           ${TESTOPTS}


pro tip: set your editor to always have a last newline

ldemailly · 2018-02-20T19:25:28Z

      - checkout
      - run: make submodule-sync
      - run: make init
+      - run: make git.pullmaster


still different order...

costinm · 2018-02-20T22:47:19Z

/retest

…uild0218

costinm · 2018-02-21T00:22:56Z

/retest

ldemailly · 2018-02-21T01:46:05Z

 install/kubernetes/istio.yaml
 samples/bookinfo/consul/bookinfo.sidecars.yaml
 samples/bookinfo/eureka/bookinfo.sidecars.yaml
+istio.VERSION


I realize I mentioned to add it to git ignore but I am hoping it still is editable despite that - the bot still updates the proxy sha in there I think

ldemailly

looks like you have an accidental change to vendor submodule, please revert that (I guess that's what jnr was worried about)

ldemailly · 2018-02-21T01:46:45Z

+# Note: in a branch, this will get the latest from master. In master it has no effect.
+# (pull == fetch + merge )
+git.pullmaster:
+	git merge master


do you need a git fetch then now ? also maybe update the comment (pull == fetch + merge ) part

ldemailly · 2018-02-21T01:58:00Z

looks like the vendor change is from #3214 so maybe if this is ready to go we can fix it as part of this PR

edit: fixed the vendor on master 57a9d1a

ldemailly · 2018-02-21T02:03:11Z

can you make pull and update the PR, I want to confirm it won't have any vendor changes

…uild0218

costinm · 2018-02-21T04:20:22Z

Did make pull, pushed. Re. .gitignore - I don't know how it'll impact the bot, but every developer testing will have the istio.VERSION updated by updateVersion.sh - and will have to remember to revert that before sumitting. It can still be submitted - with "-f". Hopefully the other PR on helm and the work to switch off updateVersion will be done soon.

…

On Tue, Feb 20, 2018 at 6:03 PM, Laurent Demailly ***@***.***> wrote: can you make pull and update the PR, I want to confirm it won't have any vendor changes — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#3599 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAFI6v3nFZpM7E2vPkoSQJvvKma5yVw2ks5tW3lggaJpZM4SJ0E1> .

ldemailly · 2018-02-21T07:45:05Z

the errors are getting fixed in mandar's pr #3638

/lgtm

but will need to / retest once that's fixed

istio-merge-robot · 2018-02-21T07:46:06Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ldemailly

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~~ [ldemailly]

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

ldemailly · 2018-02-21T08:10:50Z

/test istio-presubmit
/test istio-pilot-e2e

istio-merge-robot · 2018-02-21T09:25:37Z

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

istio-merge-robot · 2018-02-21T09:47:14Z

Automatic merge from submit-queue.

[solo-io/istio merge job] Nightly merge of upstream into master-solo

costinm added 2 commits February 18, 2018 09:26

Updates and improvements to ci

5e741b2

Adjust labels to allow upgrade/downgrade tests

2c24f52

costinm requested review from a team and ldemailly February 18, 2018 19:36

googlebot added the cla: yes label Feb 18, 2018

Revert version, and add it to ignore

570f769

ldemailly reviewed Feb 18, 2018

View reviewed changes

costinm added 2 commits February 18, 2018 12:33

Merge branch 'master' of https://github.com/istio/istio into costin-b…

a770a43

…uild0218

Fix the initialization so it works with pre-setup system

1ca93b2

costinm requested a review from a team February 19, 2018 05:13

Attempt to optimize checkout

debb7a7

costinm requested a review from a team February 19, 2018 05:41

costinm added 11 commits February 18, 2018 21:43

Attempt to optimize checkout

819af2a

Add more debug

8c4b525

Fix persist step

46feb77

Attempt to fix deps and add a build cache

c9a273d

Fix key

81d41e8

Fix key

6cfb8be

Add cache to test/lint

6735427

Route rules still failing, attempting to get more logs

4c6b20d

Move the pull and added comments. Continue to investigate caching

5261050

Adjust location of junit-report

53d7163

Review comments

20e99ff

ldemailly reviewed Feb 19, 2018

View reviewed changes

costinm added 3 commits February 19, 2018 09:15

Missed 'make'

aabe290

Try to use merge, pull wants username

553bd38

Review feedback

341457e

ldemailly reviewed Feb 20, 2018

View reviewed changes

costinm mentioned this pull request Feb 20, 2018

bin/testEnvLocalK8S.sh doesn't print URLs and doesn't use curl -f #3231

Closed

Reorder

27fd23a

costinm added 2 commits February 20, 2018 14:54

Merge branch 'master' of https://github.com/istio/istio into costin-b…

ed84096

…uild0218

Revert renaming, maybe prow doesn't like it

a2fc9c9

ldemailly reviewed Feb 21, 2018

View reviewed changes

ldemailly suggested changes Feb 21, 2018

View reviewed changes

ldemailly mentioned this pull request Feb 21, 2018

Add Group/Version per schema #3214

Merged

ldemailly approved these changes Feb 21, 2018

View reviewed changes

costinm added 2 commits February 20, 2018 20:16

Merge branch 'master' of https://github.com/istio/istio into costin-b…

1f1bf25

…uild0218

Update comments

0920660

istio-testing assigned ldemailly Feb 21, 2018

istio-testing added the lgtm label Feb 21, 2018

istio-merge-robot added the approved label Feb 21, 2018

istio-merge-robot merged commit 3cd34d8 into master Feb 21, 2018

ldemailly deleted the costin-build0218 branch February 21, 2018 10:00

PetrMc pushed a commit to PetrMc/istio that referenced this pull request Jan 14, 2026

Merge pull request istio#3599 from solo-io/nightly/merge/master-solo

a99b7e6

[solo-io/istio merge job] Nightly merge of upstream into master-solo

		@@ -1,13 +1,13 @@
		# DO NOT EDIT THIS FILE MANUALLY instead use

Conversation

costinm commented Feb 18, 2018

Uh oh!

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

costinm commented Feb 20, 2018

Uh oh!

costinm commented Feb 21, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ldemailly left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ldemailly commented Feb 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ldemailly commented Feb 21, 2018

Uh oh!

costinm commented Feb 21, 2018 via email

Uh oh!

ldemailly commented Feb 21, 2018

Uh oh!

istio-merge-robot commented Feb 21, 2018

Uh oh!

ldemailly commented Feb 21, 2018

Uh oh!

istio-merge-robot commented Feb 21, 2018

Uh oh!

istio-merge-robot commented Feb 21, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

ldemailly commented Feb 21, 2018 •

edited

Loading