Skip to content

Conversation

vadimeisenbergibm
Copy link
Contributor

  1. extract a common constant
  2. let the routing rules reside in different directories (required for tests for add istio.io tutorial yamls #3940)

@vadimeisenbergibm vadimeisenbergibm requested a review from a team March 11, 2018 19:53
@istio-merge-robot
Copy link

[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.

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

@vadimeisenbergibm
Copy link
Contributor Author

/test istio-unit-tests

@codecov
Copy link

codecov bot commented Mar 11, 2018

Codecov Report

Merging #4185 into master will increase coverage by 3%.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff            @@
##           master   #4185     +/-   ##
========================================
+ Coverage      71%     73%     +3%     
========================================
  Files         318     313      -5     
  Lines       29478   28465   -1013     
========================================
+ Hits        20649   20755    +106     
+ Misses       7584    6428   -1156     
- Partials     1245    1282     +37
Impacted Files Coverage Δ
security/pkg/pki/util/verify_cert.go 50% <0%> (-46%) ⬇️
pilot/pkg/serviceregistry/kube/cache.go 63% <0%> (-37%) ⬇️
mixer/pkg/api/grpcServer.go 74% <0%> (-26%) ⬇️
mixer/pkg/attribute/emptyBag.go 75% <0%> (-25%) ⬇️
...olarwinds/internal/papertrail/papertrail_logger.go 59% <0%> (-21%) ⬇️
pilot/pkg/config/aggregate/config.go 78% <0%> (-16%) ⬇️
mixer/pkg/attribute/protoBag.go 86% <0%> (-14%) ⬇️
...rity/cmd/node_agent_k8s/workloadapi/workloadapi.go 42% <0%> (-12%) ⬇️
mixer/adapter/prometheus/prometheus.go 80% <0%> (-9%) ⬇️
mixer/pkg/attribute/mutableBag.go 91% <0%> (-9%) ⬇️
... and 70 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 d480a68...6dac819. Read the comment docs.

@vadimeisenbergibm vadimeisenbergibm changed the title Refactor e2e bookinfo tests [WIP] Refactor e2e bookinfo tests Mar 11, 2018
@vadimeisenbergibm
Copy link
Contributor Author

Need to rethink this.

@vadimeisenbergibm
Copy link
Contributor Author

/test istio-pilot-e2e

@vadimeisenbergibm
Copy link
Contributor Author

/test istio-unit-tests

@vadimeisenbergibm
Copy link
Contributor Author

/retest

@vadimeisenbergibm
Copy link
Contributor Author

/test istio-unit-tests

@vadimeisenbergibm
Copy link
Contributor Author

/test istio-unit-tests

@vadimeisenbergibm vadimeisenbergibm changed the title [WIP] Refactor e2e bookinfo tests Refactor e2e bookinfo tests Mar 12, 2018
@vadimeisenbergibm
Copy link
Contributor Author

@ldemailly could you please review and approve? The codecov/project fails, unclear why. codecov/patch succeeds with no change in the coverage.

@ldemailly
Copy link
Member

/test e2e-rbac-auth

bookinfoDbYaml = "samples/bookinfo/kube/bookinfo-db.yaml"
bookinfoMysqlYaml = "samples/bookinfo/kube/bookinfo-mysql.yaml"
bookinfoDetailsExternalServiceYaml = "samples/bookinfo/kube/bookinfo-details-v2.yaml"
bookinfoSampleDir = "samples/bookinfo"
Copy link
Member

Choose a reason for hiding this comment

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

if this is still a constant, which it is, I'm a bit unsure what the change does
(I can agree it'd would have been cleaner to have the common prefix separated vs copypasted, but the end result is still the same... so ... what does this part of the change do ?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a minor refactoring. The change prevents typing errors in the future files to be added:

bookinfoNewYaml = "samles/bookinfo/kube/bookinfo.yaml"

vs.

bookinfoNewYaml = bookinfoSamleDir + "/kube/bookinfo.yaml"

Both code snippets have a typo, p is missing in sample.

In the first case, it will not be caught by the compiler and will fail in the test.
In the second case, it will be caught by the compiler, which is better.

Copy link
Member

Choose a reason for hiding this comment

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

except if you had a typo in

bookinfoSampleDir                  = "samles/bookinfo"

it won't either... but hopefully all the tests would

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, but bookinfoSampleDir you define only once. In the first case, you risk to make a typo in any new yaml you add.

Copy link
Member

Choose a reason for hiding this comment

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

if you use the constant in the code to join the dir with the files, it's also only once; either 100% correct or 100% wrong

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yep, this is what I did after your proposal - 4615a99

testMysqlRule = "route-rule-ratings-mysql.yaml"
detailsExternalServiceRouteRule = "route-rule-details-v2.yaml"
detailsExternalServiceEgressRule = "egress-rule-google-apis.yaml"
allRule = bookinfoSampleDir + "/kube/route-rule-all-v1.yaml"
Copy link
Member

Choose a reason for hiding this comment

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

I guess this is the real change, for rules

can you put the concatenation once in the code (ie change rulesDir or equiv) rather than add more hardcoded prefixes in all those contants, that seems going in the reverse direction (more hardcoded prefixes rather than less) ?

Copy link
Contributor Author

@vadimeisenbergibm vadimeisenbergibm Mar 13, 2018

Choose a reason for hiding this comment

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

Correct, this is the real change, to allow bookinfo yamls to reside in different directories.

can you put the concatenation once in the code

Good point, will do.

@ldemailly ldemailly requested a review from frankbu March 13, 2018 03:10
@vadimeisenbergibm
Copy link
Contributor Author

/test istio-pilot-e2e

3 similar comments
@vadimeisenbergibm
Copy link
Contributor Author

/test istio-pilot-e2e

@vadimeisenbergibm
Copy link
Contributor Author

/test istio-pilot-e2e

@vadimeisenbergibm
Copy link
Contributor Author

/test istio-pilot-e2e

@vadimeisenbergibm
Copy link
Contributor Author

/test istio-unit-tests

@vadimeisenbergibm
Copy link
Contributor Author

/test istio-pilot-e2e

@vadimeisenbergibm
Copy link
Contributor Author

@ldemailly Could you please approve this PR?

@ldemailly
Copy link
Member

/lgtm

@ldemailly ldemailly merged commit 1a61e3c into istio:master Mar 20, 2018
vadimeisenbergibm added a commit to vadimeisenbergibm/istio that referenced this pull request Apr 1, 2018
sebastienvas pushed a commit that referenced this pull request Apr 4, 2018
bianpengyuan pushed a commit to bianpengyuan/istio that referenced this pull request Apr 5, 2018
PiotrSikora added a commit to PiotrSikora/istio that referenced this pull request Aug 23, 2018
This is far from finished, but it reduces memory usage by ~10%.

Pulling the following changes from github.com/istio/proxy:

2656f34 Update Envoy SHA to latest with MetricImpl optimizations (release-1.0). (istio#1939)

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

c1cc68dda stats: refactoring MetricImpl without strings (istio#4190)
36809d80a fuzz: coverage profile generation instructions. (istio#4193)
ba40cc933 upstream: rebuild cluster when health check config is changed (istio#4075)
05c0d52d3 build: use clang-6.0. (istio#4168)
01f403ec4 thrift_proxy: introduce header transport (istio#4082)
564d256fb tcp: allow connection pool callers to store protocol state (istio#4131)
3e1d643b9 configs: match latest API changes (istio#4185)
bc6a10c2f Fix wrong mock function name. (istio#4187)
e994c1c0b Bump yaml-cpp so it builds with Visual Studio 15.8 (istio#4182)
3d1325e89 Converting envoy configs to V2 (istio#2957)
8d0680feb Add timestamp to HealthCheckEvent definition (istio#4119)
497efb95b server: handle non-EnvoyExceptions safely if thrown in constructor. (istio#4173)
6d6fafdb3 config: strengthen validation for gRPC config sources. (istio#4171)
132302caf fuzz: reduce log level when running under fuzz engine. (istio#4161)
7c04ac255 test: fix V6EmptyOptions in coverage with IPv6 enable (istio#4155)
1b2219bd7 ci: remove deprecated bazel --batch option (istio#4166)
2db6a4ce1 ci: update clang to version 6.0 in the Ubuntu build image. (istio#4157)
71152b710 ratelimit: Add ratelimit custom response headers (istio#4015)
306287418 ssl: make Ssl::Connection const everywhere (istio#4179)
706e26238 Handle validation of non expiring tokens in jwt_authn filter (istio#4007)
f06e9588a fuzz: tag trivial fuzzers with no_fuzz. (istio#4156)
27fb1d353 thrift_proxy: add service name matching to router implementation (istio#4130)
8c189a552 Make over provisioning factor configurable (istio#4003)
6c08fb43c Making test less flaky (istio#4149)
592775b7b fuzz: bare bones HCM fuzzer. (istio#4118)

For istio#7912.

Signed-off-by: Piotr Sikora <piotrsikora@google.com>
PiotrSikora added a commit to PiotrSikora/istio that referenced this pull request Aug 23, 2018
This is far from finished, but it reduces memory usage by ~10%.

Pulling the following changes from github.com/istio/proxy:

c9d2230 Update Envoy SHA to latest with MetricImpl optimizations. (istio#1938)
a32a587 Add a check cache test for string map sub keys (istio#1931)

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

c1cc68dda stats: refactoring MetricImpl without strings (istio#4190)
36809d80a fuzz: coverage profile generation instructions. (istio#4193)
ba40cc933 upstream: rebuild cluster when health check config is changed (istio#4075)
05c0d52d3 build: use clang-6.0. (istio#4168)
01f403ec4 thrift_proxy: introduce header transport (istio#4082)
564d256fb tcp: allow connection pool callers to store protocol state (istio#4131)
3e1d643b9 configs: match latest API changes (istio#4185)
bc6a10c2f Fix wrong mock function name. (istio#4187)
e994c1c0b Bump yaml-cpp so it builds with Visual Studio 15.8 (istio#4182)
3d1325e89 Converting envoy configs to V2 (istio#2957)
8d0680feb Add timestamp to HealthCheckEvent definition (istio#4119)
497efb95b server: handle non-EnvoyExceptions safely if thrown in constructor. (istio#4173)
6d6fafdb3 config: strengthen validation for gRPC config sources. (istio#4171)
132302caf fuzz: reduce log level when running under fuzz engine. (istio#4161)
7c04ac255 test: fix V6EmptyOptions in coverage with IPv6 enable (istio#4155)
1b2219bd7 ci: remove deprecated bazel --batch option (istio#4166)
2db6a4ce1 ci: update clang to version 6.0 in the Ubuntu build image. (istio#4157)
71152b710 ratelimit: Add ratelimit custom response headers (istio#4015)
306287418 ssl: make Ssl::Connection const everywhere (istio#4179)
706e26238 Handle validation of non expiring tokens in jwt_authn filter (istio#4007)
f06e9588a fuzz: tag trivial fuzzers with no_fuzz. (istio#4156)
27fb1d353 thrift_proxy: add service name matching to router implementation (istio#4130)
8c189a552 Make over provisioning factor configurable (istio#4003)
6c08fb43c Making test less flaky (istio#4149)
592775b7b fuzz: bare bones HCM fuzzer. (istio#4118)

For istio#7912.

Signed-off-by: Piotr Sikora <piotrsikora@google.com>
rshriram pushed a commit that referenced this pull request Aug 23, 2018
…). (#8161)

This is far from finished, but it reduces memory usage by ~10%.

Pulling the following changes from github.com/istio/proxy:

2656f34 Update Envoy SHA to latest with MetricImpl optimizations (release-1.0). (#1939)

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

c1cc68dda stats: refactoring MetricImpl without strings (#4190)
36809d80a fuzz: coverage profile generation instructions. (#4193)
ba40cc933 upstream: rebuild cluster when health check config is changed (#4075)
05c0d52d3 build: use clang-6.0. (#4168)
01f403ec4 thrift_proxy: introduce header transport (#4082)
564d256fb tcp: allow connection pool callers to store protocol state (#4131)
3e1d643b9 configs: match latest API changes (#4185)
bc6a10c2f Fix wrong mock function name. (#4187)
e994c1c0b Bump yaml-cpp so it builds with Visual Studio 15.8 (#4182)
3d1325e89 Converting envoy configs to V2 (#2957)
8d0680feb Add timestamp to HealthCheckEvent definition (#4119)
497efb95b server: handle non-EnvoyExceptions safely if thrown in constructor. (#4173)
6d6fafdb3 config: strengthen validation for gRPC config sources. (#4171)
132302caf fuzz: reduce log level when running under fuzz engine. (#4161)
7c04ac255 test: fix V6EmptyOptions in coverage with IPv6 enable (#4155)
1b2219bd7 ci: remove deprecated bazel --batch option (#4166)
2db6a4ce1 ci: update clang to version 6.0 in the Ubuntu build image. (#4157)
71152b710 ratelimit: Add ratelimit custom response headers (#4015)
306287418 ssl: make Ssl::Connection const everywhere (#4179)
706e26238 Handle validation of non expiring tokens in jwt_authn filter (#4007)
f06e9588a fuzz: tag trivial fuzzers with no_fuzz. (#4156)
27fb1d353 thrift_proxy: add service name matching to router implementation (#4130)
8c189a552 Make over provisioning factor configurable (#4003)
6c08fb43c Making test less flaky (#4149)
592775b7b fuzz: bare bones HCM fuzzer. (#4118)

For #7912.

Signed-off-by: Piotr Sikora <piotrsikora@google.com>
istio-testing pushed a commit that referenced this pull request Aug 24, 2018
This is far from finished, but it reduces memory usage by ~10%.

Pulling the following changes from github.com/istio/proxy:

c9d2230 Update Envoy SHA to latest with MetricImpl optimizations. (#1938)
a32a587 Add a check cache test for string map sub keys (#1931)

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

c1cc68dda stats: refactoring MetricImpl without strings (#4190)
36809d80a fuzz: coverage profile generation instructions. (#4193)
ba40cc933 upstream: rebuild cluster when health check config is changed (#4075)
05c0d52d3 build: use clang-6.0. (#4168)
01f403ec4 thrift_proxy: introduce header transport (#4082)
564d256fb tcp: allow connection pool callers to store protocol state (#4131)
3e1d643b9 configs: match latest API changes (#4185)
bc6a10c2f Fix wrong mock function name. (#4187)
e994c1c0b Bump yaml-cpp so it builds with Visual Studio 15.8 (#4182)
3d1325e89 Converting envoy configs to V2 (#2957)
8d0680feb Add timestamp to HealthCheckEvent definition (#4119)
497efb95b server: handle non-EnvoyExceptions safely if thrown in constructor. (#4173)
6d6fafdb3 config: strengthen validation for gRPC config sources. (#4171)
132302caf fuzz: reduce log level when running under fuzz engine. (#4161)
7c04ac255 test: fix V6EmptyOptions in coverage with IPv6 enable (#4155)
1b2219bd7 ci: remove deprecated bazel --batch option (#4166)
2db6a4ce1 ci: update clang to version 6.0 in the Ubuntu build image. (#4157)
71152b710 ratelimit: Add ratelimit custom response headers (#4015)
306287418 ssl: make Ssl::Connection const everywhere (#4179)
706e26238 Handle validation of non expiring tokens in jwt_authn filter (#4007)
f06e9588a fuzz: tag trivial fuzzers with no_fuzz. (#4156)
27fb1d353 thrift_proxy: add service name matching to router implementation (#4130)
8c189a552 Make over provisioning factor configurable (#4003)
6c08fb43c Making test less flaky (#4149)
592775b7b fuzz: bare bones HCM fuzzer. (#4118)

For #7912.

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.

6 participants