Skip to content

remove all bazel-genfiles for every presubmit build#1697

Closed
jmuk wants to merge 4 commits intoistio:masterfrom
jmuk:presubmit
Closed

remove all bazel-genfiles for every presubmit build#1697
jmuk wants to merge 4 commits intoistio:masterfrom
jmuk:presubmit

Conversation

@jmuk
Copy link
Copy Markdown
Contributor

@jmuk jmuk commented Nov 13, 2017

Fixes #1689, all of the genfiles artifacts should be cleaned up.
This can decrease the continuous build performance, although
I hope the effect is tiny -- running codegen tools, recompiling
some files, and re-linking the result (but most of the cases
re-linking would happen anyways).

What this PR does / why we need it:

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #1689

Special notes for your reviewer:

Release note:

none

Fixes istio#1689, all of the genfiles artifacts should be cleaned up.
This can decrease the continuous build performance, although
I hope the effect is tiny -- running codegen tools, recompiling
some files, and re-linking the result (but most of the cases
re-linking would happen anyways).
@istio-merge-robot
Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
We suggest the following additional approver: sebastienvas

Assign the PR to them by writing /assign @sebastienvas in a comment when ready.

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:

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

@jmuk
Copy link
Copy Markdown
Contributor Author

jmuk commented Nov 13, 2017

@yutongz can you check the performance effect on this PR? This should fix the problem, but the effect can be small, since it just removes the autogen files only. If the performance is okay, I think this solves the problem.

@yutongz
Copy link
Copy Markdown
Contributor

yutongz commented Nov 13, 2017

Thanks, I will keep an eye on that.

@mandarjog mandarjog requested a review from nlandolfi November 13, 2017 23:56
@mandarjog mandarjog self-assigned this Nov 13, 2017
@mandarjog
Copy link
Copy Markdown
Contributor

/retest

@yutongz
Copy link
Copy Markdown
Contributor

yutongz commented Nov 14, 2017

First run failed with the same issue we had last friday and second run failed on auto-generated files.

@codecov
Copy link
Copy Markdown

codecov bot commented Nov 14, 2017

Codecov Report

Merging #1697 into master will decrease coverage by 0.02%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #1697      +/-   ##
=========================================
- Coverage   81.22%   81.2%   -0.03%     
=========================================
  Files         191     191              
  Lines       19455   19455              
=========================================
- Hits        15803   15799       -4     
- Misses       3192    3194       +2     
- Partials      460     462       +2
Flag Coverage Δ
#broker 45.51% <ø> (ø) ⬆️
#mixer 82.45% <ø> (ø) ⬆️
#pilot 80.5% <ø> (-0.06%) ⬇️
#security 90.39% <ø> (ø) ⬆️
Impacted Files Coverage Δ
pilot/platform/kube/queue.go 82.69% <0%> (-7.7%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d10410a...a208dba. Read the comment docs.

@nlandolfi
Copy link
Copy Markdown
Contributor

Yeah, as Yutong said it looks like most recent failed on autogenerated files. could these have diverged back when the check was removed?

@jmuk
Copy link
Copy Markdown
Contributor Author

jmuk commented Nov 14, 2017

Very weird, I have no idea how and where those mixer/template/authz come from, since this does not happen locally and no one is referring it. Let me look into the details.

@yutongz
Copy link
Copy Markdown
Contributor

yutongz commented Nov 14, 2017

/test istio-presubmit

I cleaned the Prow cache, let try it again.

@jmuk
Copy link
Copy Markdown
Contributor Author

jmuk commented Nov 14, 2017

/retest

@yutongz
Copy link
Copy Markdown
Contributor

yutongz commented Nov 14, 2017

There is something wrong in master breaking presubmit for now.

@yutongz
Copy link
Copy Markdown
Contributor

yutongz commented Nov 14, 2017

/retest

1 similar comment
@jmuk
Copy link
Copy Markdown
Contributor Author

jmuk commented Nov 14, 2017

/retest

@istio-merge-robot
Copy link
Copy Markdown

@jmuk PR needs rebase

@istio-merge-robot istio-merge-robot removed the needs-rebase Indicates a PR needs to be rebased before being merged label Nov 29, 2017
@jmuk
Copy link
Copy Markdown
Contributor Author

jmuk commented Nov 29, 2017

/retest

@jmuk
Copy link
Copy Markdown
Contributor Author

jmuk commented Dec 2, 2017

this is not necessary anymore, closing.

@jmuk jmuk closed this Dec 2, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

presubmit can fail randomly for nonexistent autogen files

7 participants