Enable presubmit on Prow by sebastienvas · Pull Request #464 · istio/istio

sebastienvas · 2017-07-10T22:02:50Z

Fix Build Issues
Create presubmit script
Update e2e framework to save logs to _artifacts folder instead of uplading them to gcloud storage since Prow already does this.

istio-testing · 2017-07-10T22:02:58Z

Hi @sebastienvas. Thanks for your PR.

I'm waiting for a member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

I understand the commands that are listed here.

Details

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-10T22:05:27Z

/ok-to-test

istio-testing · 2017-07-10T22:05:41Z

@sebastienvas: you can't request testing unless you are a member.

Details

In response to this:

/ok-to-test

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.

istio-testing · 2017-07-10T22:30:10Z

Jenkins job istio/presubmit passed

istio-testing · 2017-07-10T22:31:11Z

Jenkins job istio/presubmit passed

istio-testing · 2017-07-10T22:32:03Z

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

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

Details

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.

nlandolfi · 2017-07-10T23:13:22Z

prow/istio-presubmit.sh

I believe this should be --test_log_path, note no plural. based on the build log here

istio-testing · 2017-07-11T04:50:40Z

Jenkins job istio/presubmit passed

istio-testing · 2017-07-11T17:48:07Z

Jenkins job istio/presubmit passed

istio-testing · 2017-07-11T20:14:33Z

Jenkins job istio/presubmit passed

istio-testing · 2017-07-11T20:17:15Z

Jenkins job istio/presubmit passed

sebastienvas · 2017-07-11T21:38:07Z

Prow results are available here

istio-testing · 2017-07-11T22:21:03Z

Jenkins job istio/presubmit passed

istio-testing · 2017-07-11T23:44:45Z

Jenkins job istio/presubmit passed

sebastienvas · 2017-07-12T00:04:09Z

Prow results are here. Note that artifacts were also uploaded as well.

istio-testing · 2017-07-12T00:06:39Z

Jenkins job istio/presubmit passed

nlandolfi

LGTM

nlandolfi · 2017-07-12T03:32:18Z

prow/istio-presubmit.sh

not as familiar with this syntax but basically this allows running the script locally? like you define the e2e args before the if for that purpose?

Yes. Precisely.

oh I missed E2E_ARGS here (search fail!)

nlandolfi · 2017-07-12T03:33:28Z

tests/e2e/framework/testInfo.go

ahh i see the original typo came from that the var is testLogsPath whereas flag is test_log_path

I renamed it for consistency.

istio-testing · 2017-07-12T19:55:18Z

Jenkins job istio/presubmit passed

istio-testing · 2017-07-12T23:11:54Z

Jenkins job istio/presubmit passed

ldemailly

couple of issues/comments/questions:

ldemailly · 2017-07-14T00:05:02Z

prow/istio-presubmit.sh

@@ -0,0 +1,28 @@
+#!/bin/bash
+


a short description of what this does and how maybe would be a useful comment/header ?

ldemailly · 2017-07-14T00:06:25Z

prow/istio-presubmit.sh

+
+if [ "${CI}" == "bootstrap" ]; then
+    # ensure correct path
+    mkdir -p ${GOPATH}/src/istio.io


test if GOPATH is set first ?

GOPATH is set in the docker image. https://github.com/istio/test-infra/blob/master/docker/prowbazel/Dockerfile#L43

Added set -u.

ldemailly · 2017-07-14T00:07:24Z

prow/istio-presubmit.sh

+    ln -s ${GOPATH}/src/github.com/istio/istio ${GOPATH}/src/istio.io
+    cd ${GOPATH}/src/istio.io/istio/
+    ARTIFACTS_DIR="${GOPATH}/src/istio.io/istio/_artifacts"
+    E2E_ARGS+=(--test_logs_path="${ARTIFACTS_DIR}")


I don't see an export or those 2 variables being used anywhere ? (only 'bug' if I read this correctly/am not missing something)

E2E_ARGS is used down in the script.

ldemailly · 2017-07-14T00:09:50Z

tests/e2e/framework/testInfo.go

-		if err != nil {
+	tmpDir := ""
+	// testLogsPath will be used when called by Prow.
+	// Bootstrap already gather stdout and stdin so we don't need to keep the logs from glog.


are we using -logtostderr for glog then ?

Yes. This is is in e2e.sh. https://github.com/istio/istio/blob/master/tests/e2e.sh#L19

sebastienvas

PTAL

istio-testing · 2017-07-14T16:46:53Z

Jenkins job istio/presubmit passed

ldemailly

thx!

nlandolfi · 2017-07-15T03:52:59Z

bam!

…

On Fri, Jul 14, 2017 at 7:48 PM Sebastien Vas ***@***.***> wrote: Merged #464 <#464>. — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub <#464 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ACBRURzOSeYvChP9SVPA7wNp4Y4IkC_9ks5sN_48gaJpZM4OTech> .

* Enable Prow * Fix BUILD format * Add execution perm to the script * Fix shell script * Fix flag name * Use the right k8s cluster * Create the ~.kube dir * Linking instead of moving * Explicitly specify dir * Remove debug statements * Rename test_log_path to test_logs_path for consistency * Adds desc + failure conditions Former-commit-id: 148a5a3

jaeger-operator update image to latest 1.43.0 and bump chart version

Added mechanism to skip test suites

sebastienvas requested a review from nlandolfi July 10, 2017 22:02

googlebot added the cla: yes label Jul 10, 2017

istio-testing added the needs-ok-to-test label Jul 10, 2017

istio-testing removed the needs-ok-to-test label Jul 10, 2017

sebastienvas force-pushed the prow branch from d2470b3 to bd4fff1 Compare July 10, 2017 22:24

nlandolfi reviewed Jul 10, 2017

View reviewed changes

nlandolfi mentioned this pull request Jul 11, 2017

make istio/istio prow silent for now istio/test-infra#270

Merged

nlandolfi approved these changes Jul 12, 2017

View reviewed changes

sebastienvas added 7 commits July 12, 2017 12:35

Enable Prow

f808789

Fix BUILD format

d17c53f

Add execution perm to the script

786cca5

Fix shell script

80b0130

Fix flag name

3c6e4bd

Use the right k8s cluster

7ea04a5

Create the ~.kube dir

3ab6486

sebastienvas added 4 commits July 12, 2017 12:35

Linking instead of moving

87c0150

Explicitly specify dir

aefa2bb

Remove debug statements

da11ac6

Rename test_log_path to test_logs_path for consistency

96c6288

sebastienvas force-pushed the prow branch from 7775e33 to 96c6288 Compare July 12, 2017 19:35

sebastienvas requested a review from ldemailly July 13, 2017 18:29

ldemailly reviewed Jul 14, 2017

View reviewed changes

Adds desc + failure conditions

02d136f

sebastienvas commented Jul 14, 2017

View reviewed changes

ldemailly approved these changes Jul 14, 2017

View reviewed changes

sebastienvas merged commit 148a5a3 into istio:master Jul 14, 2017

sebastienvas deleted the prow branch July 14, 2017 23:48

sebastienvas mentioned this pull request Jul 14, 2017

Fetch, save, and upload cluster trace logs from multiple services #440

Merged

kyessenov pushed a commit to kyessenov/istio that referenced this pull request Aug 13, 2018

rename target to destination (istio#464)

892d76c

antonioberben pushed a commit to antonioberben/istio that referenced this pull request Jan 29, 2024

Merge pull request istio#464 from christopheroconnell/main

30fb3ba

jaeger-operator update image to latest 1.43.0 and bump chart version

fjglira pushed a commit to fjglira/istio that referenced this pull request Sep 26, 2025

Merge pull request istio#464 from ctartici/master

44c062e

Added mechanism to skip test suites

Conversation

sebastienvas commented Jul 10, 2017

Uh oh!

istio-testing commented Jul 10, 2017

Uh oh!

sebastienvas commented Jul 10, 2017

Uh oh!

istio-testing commented Jul 10, 2017

Uh oh!

istio-testing commented Jul 10, 2017

Uh oh!

istio-testing commented Jul 10, 2017

Uh oh!

istio-testing commented Jul 10, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

istio-testing commented Jul 11, 2017

Uh oh!

istio-testing commented Jul 11, 2017

Uh oh!

istio-testing commented Jul 11, 2017

Uh oh!

istio-testing commented Jul 11, 2017

Uh oh!

sebastienvas commented Jul 11, 2017

Uh oh!

istio-testing commented Jul 11, 2017

Uh oh!

istio-testing commented Jul 11, 2017

Uh oh!

sebastienvas commented Jul 12, 2017

Uh oh!

istio-testing commented Jul 12, 2017

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!

istio-testing commented Jul 12, 2017

Uh oh!

istio-testing commented Jul 12, 2017

Uh oh!

ldemailly left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

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 Jul 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sebastienvas left a comment

Choose a reason for hiding this comment

ldemailly left a comment •

edited

Loading

ldemailly Jul 14, 2017 •

edited

Loading

sebastienvas Jul 14, 2017 •

edited

Loading