Skip to content

Conversation

ZackButcher
Copy link
Contributor

What this PR does / why we need it: The name svcctrl is not a good golang package name for a variety of reasons, most significant of which is it's not obvious what this adapter is reading through the directories under //mixer/adapter. This PR renames it to use the full name.

Release note:

NONE

@@ -9,6 +9,9 @@ mixer/adapter/kubernetes/config/config.pb.go
mixer/adapter/list/config/config.pb.go
mixer/adapter/memquota/config/config.pb.go
mixer/adapter/prometheus/config/config.pb.go
mixer/adapter/servicecontrol/config/config.pb.go
mixer/adapter/servicecontrol/template/servicecontrolreport/go_default_library_handler.gen.go
mixer/adapter/servicecontrol/template/servicecontrolreport/go_default_library_tmpl.pb.go
Copy link
Contributor

Choose a reason for hiding this comment

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

it's weird genfiles under mixer/adapter/svcctrl are not removed from this file -- this is actually yet another breakage of bazel_to_go.py.

For now, please run bazel clean, and then bazel build //... and bin/bazel_to_go.py. That will remove those old file entries.

@codecov
Copy link

codecov bot commented Nov 29, 2017

Codecov Report

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

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1922      +/-   ##
==========================================
- Coverage   78.06%   70.76%   -7.31%     
==========================================
  Files          76       21      -55     
  Lines        7377      667    -6710     
==========================================
- Hits         5759      472    -5287     
+ Misses       1302      166    -1136     
+ Partials      316       29     -287
Flag Coverage Δ
#broker 47.27% <ø> (ø) ⬆️
#pilot ?
#security 87.24% <ø> (ø) ⬆️
Impacted Files Coverage Δ
pilot/platform/kube/queue.go
pilot/adapter/config/memory/config.go
pilot/model/service.go
pilot/model/validation.go
pilot/proxy/envoy/watcher.go
pilot/adapter/config/crd/client.go
pilot/proxy/envoy/ingress.go
pilot/proxy/net.go
pilot/proxy/envoy/infra_auth.go
pilot/proxy/resolve.go
... and 45 more

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 274fd4b...be1dff2. Read the comment docs.

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

@ZackButcher PR needs rebase

@ZackButcher
Copy link
Contributor Author

This stuff with the generated files breaking presubmit must be fixed, this is ridiculous.

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

kyessenov commented Nov 29, 2017 via email

@ZackButcher
Copy link
Contributor Author

I'd be happy to if I knew what was even generating the files. Unfortunately, I can't repro locally (you know, cause then this wouldn't be an issue, since the files would exist).

@ZackButcher
Copy link
Contributor Author

I'm more than happy to rip those checks out of the presubmit, but I think that'd wind up with us in an even worse spot.

@ZackButcher
Copy link
Contributor Author

This failure is due to #1882 and the fact that files persist across runs on our presubmit machines, which is why this was impossible to repro locally.

@guptasu
Copy link
Contributor

guptasu commented Nov 29, 2017

aha. bazel caching. Should we just do bazel clean before doing anything to work around for now ?

@kyessenov
Copy link
Contributor

kyessenov commented Nov 29, 2017 via email

@ZackButcher
Copy link
Contributor Author

Our presubmit pipeline being hermetic is significantly more important than them taking a few minutes longer to run. We need correctness first, then we can worry about performance.

@guptasu
Copy link
Contributor

guptasu commented Nov 29, 2017

Another alternative is when we check for dirty files and break the presubmit, we should skip all the generated file .gen.go and the .pb.go. WDYT @jmuk @ayj @ZackButcher @kyessenov

@ZackButcher
Copy link
Contributor Author

/retest

@douglas-reid
Copy link
Contributor

/lgtm

@istio-merge-robot
Copy link

@ZackButcher PR needs rebase

@istio-merge-robot istio-merge-robot added the needs-rebase Indicates a PR needs to be rebased before being merged label Nov 30, 2017
@istio-merge-robot istio-merge-robot removed the needs-rebase Indicates a PR needs to be rebased before being merged label Dec 11, 2017
@ZackButcher
Copy link
Contributor Author

PTAL

@ZackButcher ZackButcher requested a review from geeknoid December 13, 2017 19:30
@manlinl
Copy link
Contributor

manlinl commented Dec 13, 2017

sorry was busy with a demo. I will take a look this afternoon.

@manlinl
Copy link
Contributor

manlinl commented Dec 13, 2017

/lgtm

@istio-merge-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: douglas-reid, manlinl

The full list of commands accepted by this bot can be found here.

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

@istio-merge-robot
Copy link

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

@istio-merge-robot
Copy link

Automatic merge from submit-queue.

@istio-merge-robot istio-merge-robot merged commit a598a62 into istio:master Dec 13, 2017
@ZackButcher ZackButcher deleted the service-control branch December 14, 2017 20:29
PiotrSikora added a commit to PiotrSikora/istio that referenced this pull request Aug 21, 2018
Pulling the following changes from github.com/istio/proxy:

1fc6253 add debug logs for collecting rbac attributes (istio#1922)
c5282b6 Update Envoy SHA to latest with LcTrie optimizations. (istio#1918)
4ced9e7 Update clang to 6.0 and use it for release binaries. (istio#1914)
585abec fixed broken links to dev guide and contribution guide (istio#1913)
c63d841 Provide source version information in the binary. (istio#1915)
b49589a Install clang-format in the build image used by CircleCI. (istio#1917)
5d42471 Fix macOS build on CircleCI. (istio#1916)
b1f4e7e add rbac filter to istio http integration test. (istio#1907)

Pulling the following changes from github.com/envoyproxy/envoy:

73bd3d95c http_filter: add addEncodedTrailers and addDecodedTrailers (istio#3980)
c3652aad5 rbac/fuzz: fix build (istio#4150)
07bc27c05 fix flaky RBAC integration test. (istio#4147)
b150d61a9 header_map: copy constructor for HeaderMapImpl. (istio#4129)
f345c8b23 test: moving websocket tests to using HTTP codec. (istio#4143)
da500d20f upstream: init host hc value based on hc value from other priorities (istio#3959)
da6194b94 test: add tests for corner-cases around sending requests before run() starts or after run() ends. (istio#4114)
3527f7799 perf: reduce the memory usage of LC Trie construction (istio#4117)
b538e46d8 test: moving redundant code in websocket_integration_test to utilities (istio#4127)
a3c55bf7b test: make YamlLoadFromStringFail less picky about error msg. (istio#4141)
c283439b6 rbac: add rbac network filter. (istio#4083)
5a7152d21 fuzz: route lookup and header finalization fuzzer. (istio#4116)
589467360 Set content-type and content-length (istio#4113)
714ae130a fault: use FractionalPercent for percent (istio#3978)
fde378705 test: Fix inverted exact match logic in IntegrationTcpClient::waitForData() (istio#4134)
794a00126 Added cluster_name to load assignment config for static cluster (istio#4123)
19f51e5e1 ssl: refactor ContextConfig to use TlsCertificateConfig (istio#4115)
0a4bffc5a syscall: refactor OsSysCalls for deeper errno latching (istio#4111)
ec0d98e5e thrift_proxy: fix oneway bugs (istio#4025)
1381673ad Do not crash when converting YAML to JSON fails (istio#4110)
2662bf1f2 config: allow unknown fields flag (take 2) (istio#4096)
1ab839c1f Use a jittered backoff strategy for handling HdsDelegate stream/connection failures (istio#4108)
7309c14cf bazel: use GCS remote cache (istio#4050)
5fe4e14f0 Add thread local cache of overload action states (istio#4090)
3bb7fbc5f Added TCP healthcheck capabilities to the HdsDelegate (istio#4079)
98037ed37 secret: add secret provider interface and use it for TlsCertificates (istio#4086)
3e15c9490 upstream: allow custom extension protocol options (istio#4098)
9b33c49d1 Rename message types in hds.proto to improve readability (istio#4109)
bb70b42bb fuzz: router header formatter/parser fuzz test. (istio#4105)
fe57f6b33 fuzz: http parsing utility fuzzer. (istio#4107)
73dfedc95 ci: link ninja-buid to ninja for centos (istio#4106)
1cd509ef1 docs: add curl to Ubuntu deps (istio#4104)
45b900829 Handling updates from the management server on HDS (istio#4077)
510994c6a Don't use SIGTERM for admin /quitquitquit, just shut down directly. (istio#4099)
29b60291e fuzz: access log formatter fuzz test. (istio#4102)
765cac42f Destroy pending updates when updating a cluster (istio#4084)
aafdf6037 authz_client_fix: fixed ext_authz http client when request contains content-length greater than 0 (istio#3888)
22ae0ab93 HttpConnectionManager and upstream counters for total completed requests (istio#3995)
04616d676  tcp_proxy: convert TCP proxy to use TCP connection pool (istio#4067)
e759eab17 buffer: add prepend functions to Buffer::Instance (istio#4064)

Fixes istio#7710, fixes istio#7817, and hopefully fixes istio#7759.

Signed-off-by: Piotr Sikora <piotrsikora@google.com>
istio-testing pushed a commit that referenced this pull request Aug 22, 2018
Pulling the following changes from github.com/istio/proxy:

1fc6253 add debug logs for collecting rbac attributes (#1922)
c5282b6 Update Envoy SHA to latest with LcTrie optimizations. (#1918)
4ced9e7 Update clang to 6.0 and use it for release binaries. (#1914)
585abec fixed broken links to dev guide and contribution guide (#1913)
c63d841 Provide source version information in the binary. (#1915)
b49589a Install clang-format in the build image used by CircleCI. (#1917)
5d42471 Fix macOS build on CircleCI. (#1916)
b1f4e7e add rbac filter to istio http integration test. (#1907)

Pulling the following changes from github.com/envoyproxy/envoy:

73bd3d95c http_filter: add addEncodedTrailers and addDecodedTrailers (#3980)
c3652aad5 rbac/fuzz: fix build (#4150)
07bc27c05 fix flaky RBAC integration test. (#4147)
b150d61a9 header_map: copy constructor for HeaderMapImpl. (#4129)
f345c8b23 test: moving websocket tests to using HTTP codec. (#4143)
da500d20f upstream: init host hc value based on hc value from other priorities (#3959)
da6194b94 test: add tests for corner-cases around sending requests before run() starts or after run() ends. (#4114)
3527f7799 perf: reduce the memory usage of LC Trie construction (#4117)
b538e46d8 test: moving redundant code in websocket_integration_test to utilities (#4127)
a3c55bf7b test: make YamlLoadFromStringFail less picky about error msg. (#4141)
c283439b6 rbac: add rbac network filter. (#4083)
5a7152d21 fuzz: route lookup and header finalization fuzzer. (#4116)
589467360 Set content-type and content-length (#4113)
714ae130a fault: use FractionalPercent for percent (#3978)
fde378705 test: Fix inverted exact match logic in IntegrationTcpClient::waitForData() (#4134)
794a00126 Added cluster_name to load assignment config for static cluster (#4123)
19f51e5e1 ssl: refactor ContextConfig to use TlsCertificateConfig (#4115)
0a4bffc5a syscall: refactor OsSysCalls for deeper errno latching (#4111)
ec0d98e5e thrift_proxy: fix oneway bugs (#4025)
1381673ad Do not crash when converting YAML to JSON fails (#4110)
2662bf1f2 config: allow unknown fields flag (take 2) (#4096)
1ab839c1f Use a jittered backoff strategy for handling HdsDelegate stream/connection failures (#4108)
7309c14cf bazel: use GCS remote cache (#4050)
5fe4e14f0 Add thread local cache of overload action states (#4090)
3bb7fbc5f Added TCP healthcheck capabilities to the HdsDelegate (#4079)
98037ed37 secret: add secret provider interface and use it for TlsCertificates (#4086)
3e15c9490 upstream: allow custom extension protocol options (#4098)
9b33c49d1 Rename message types in hds.proto to improve readability (#4109)
bb70b42bb fuzz: router header formatter/parser fuzz test. (#4105)
fe57f6b33 fuzz: http parsing utility fuzzer. (#4107)
73dfedc95 ci: link ninja-buid to ninja for centos (#4106)
1cd509ef1 docs: add curl to Ubuntu deps (#4104)
45b900829 Handling updates from the management server on HDS (#4077)
510994c6a Don't use SIGTERM for admin /quitquitquit, just shut down directly. (#4099)
29b60291e fuzz: access log formatter fuzz test. (#4102)
765cac42f Destroy pending updates when updating a cluster (#4084)
aafdf6037 authz_client_fix: fixed ext_authz http client when request contains content-length greater than 0 (#3888)
22ae0ab93 HttpConnectionManager and upstream counters for total completed requests (#3995)
04616d676  tcp_proxy: convert TCP proxy to use TCP connection pool (#4067)
e759eab17 buffer: add prepend functions to Buffer::Instance (#4064)

Fixes #7710, fixes #7817, and hopefully fixes #7759.

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.

9 participants