Skip to content

consolidate scripts in bin directory in istio components#1875

Merged
rshriram merged 1 commit intoistio:masterfrom
jmuk:bin
Nov 28, 2017
Merged

consolidate scripts in bin directory in istio components#1875
rshriram merged 1 commit intoistio:masterfrom
jmuk:bin

Conversation

@jmuk
Copy link
Copy Markdown
Contributor

@jmuk jmuk commented Nov 27, 2017

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 #1844

Special notes for your reviewer:

Release note:

none

@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 approvers: geeknoid, wenchenglu

Assign the PR to them by writing /assign @geeknoid @wenchenglu 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 27, 2017

/assign @geeknoid

Copy link
Copy Markdown
Member

@rshriram rshriram left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you rename to scripts/ ?

@rshriram
Copy link
Copy Markdown
Member

And please fix circleci broker test.

@geeknoid
Copy link
Copy Markdown
Contributor

I rather have a single "bin" directory which contains everything that can be run, whether an actual binary or a script. It doesn't matter to the user of this thing, except for having the thing in their path.

@codecov
Copy link
Copy Markdown

codecov bot commented Nov 28, 2017

Codecov Report

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

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1875      +/-   ##
==========================================
- Coverage   81.23%   81.19%   -0.05%     
==========================================
  Files         191      186       -5     
  Lines       19344    18772     -572     
==========================================
- Hits        15715    15242     -473     
+ Misses       3173     3103      -70     
+ Partials      456      427      -29
Flag Coverage Δ
#broker 44.44% <ø> (-1.07%) ⬇️
#mixer 82.54% <ø> (ø) ⬆️
#pilot 80.31% <ø> (-0.17%) ⬇️
#security 90.39% <ø> (ø) ⬆️
Impacted Files Coverage Δ
pilot/platform/kube/queue.go 82.69% <0%> (-7.7%) ⬇️
pilot/platform/kube/inject/http.go
pilot/platform/kube/inject/initializer.go
pilot/platform/kube/inject/configmap.go
broker/pkg/version/version.go
pilot/platform/kube/inject/inject.go

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 85983c2...c0a4adf. Read the comment docs.

@jmuk
Copy link
Copy Markdown
Contributor Author

jmuk commented Nov 28, 2017

the broker failure looks like circleci's flake. rerun seems to fix the issue.

I don't have strong opinion on the name of directories, but it's been bin/ for each of those projects, I don't see strong reasons to be renamed now.

@rshriram rshriram merged commit f738d85 into istio:master Nov 28, 2017
vadimeisenbergibm pushed a commit to vadimeisenbergibm/istio that referenced this pull request Nov 28, 2017
@jmuk
Copy link
Copy Markdown
Contributor Author

jmuk commented Nov 28, 2017

Thank you for merging, but please don't merge changes like this manually. Some other things are merged meanwhile, which makes the presubmit fail. See #1890

@rshriram
Copy link
Copy Markdown
Member

Shouldn't those changes respect the style/lint etc. ?

kyessenov pushed a commit to kyessenov/istio that referenced this pull request Aug 13, 2018
1. Stop trashing build's cache.
2. Split ASan and TSan tests into separate targets.
3. Fix output directory, so that caching works on macOS.

Before:
- build     : 49 mins
- macos     : 30 mins

After:
- build     : 27 mins
- linux_asan: 20 mins
- linux_tsan: 16 mins
- macos     : 30 mins

After (with warm cache):
- build     :  3 mins
- linux_asan:  4 mins
- linux_tsan:  2 mins
- macos     :  5 mins

Fixes istio#1815.

Signed-off-by: Piotr Sikora <piotrsikora@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Remove any redundant stuff from */bin

6 participants