Skip to content

Remove code duplication of string to model.Protocol conversion#1866

Merged
rshriram merged 8 commits intoistio:masterfrom
vadimeisenbergibm:model_convert_protocol
Nov 27, 2017
Merged

Remove code duplication of string to model.Protocol conversion#1866
rshriram merged 8 commits intoistio:masterfrom
vadimeisenbergibm:model_convert_protocol

Conversation

@vadimeisenbergibm
Copy link
Copy Markdown
Contributor

What this PR does / why we need it:
Removes code duplication of string to model.Protocol conversion

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

Special notes for your reviewer:
This PR is needed for Mongo and Redis protocols support in Egress Rules

Release note:

@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: ldemailly

Assign the PR to them by writing /assign @ldemailly 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

@codecov
Copy link
Copy Markdown

codecov bot commented Nov 27, 2017

Codecov Report

Merging #1866 into master will decrease coverage by 0.06%.
The diff coverage is 55.88%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1866      +/-   ##
==========================================
- Coverage    81.9%   81.84%   -0.07%     
==========================================
  Files         188      189       +1     
  Lines       18715    18702      -13     
==========================================
- Hits        15328    15306      -22     
- Misses       2960     2968       +8     
- Partials      427      428       +1
Flag Coverage Δ
#broker 45.51% <ø> (+1.06%) ⬆️
#mixer 82.49% <ø> (ø) ⬆️
#pilot 82.09% <55.88%> (-0.19%) ⬇️
#security 90.39% <ø> (ø) ⬆️
Impacted Files Coverage Δ
pilot/proxy/envoy/config.go 92.18% <100%> (ø) ⬆️
pilot/platform/eureka/conversion.go 84.7% <100%> (-0.15%) ⬇️
pilot/platform/kube/conversion.go 86.18% <100%> (-0.78%) ⬇️
pilot/model/validation.go 97.1% <100%> (ø) ⬆️
pilot/model/service.go 91.97% <31.57%> (-8.03%) ⬇️
pilot/platform/consul/conversion.go 89.24% <50%> (+2.21%) ⬆️
pilot/platform/consul/monitor.go 76.19% <0%> (-3.58%) ⬇️
broker/pkg/version/version.go 100% <0%> (ø)
mixer/adapter/stackdriver/metric/bufferedClient.go 92.5% <0%> (+2.5%) ⬆️

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 cc39692...f257aa7. Read the comment docs.

@codecov
Copy link
Copy Markdown

codecov bot commented Nov 27, 2017

Codecov Report

Merging #1866 into master will decrease coverage by 0.64%.
The diff coverage is 94.11%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1866      +/-   ##
==========================================
- Coverage    81.9%   81.25%   -0.65%     
==========================================
  Files         188      176      -12     
  Lines       18715    16553    -2162     
==========================================
- Hits        15328    13450    -1878     
+ Misses       2960     2745     -215     
+ Partials      427      358      -69
Flag Coverage Δ
#broker 44.44% <ø> (ø) ⬆️
#mixer 82.48% <ø> (ø) ⬆️
#pilot 80.3% <94.11%> (-1.98%) ⬇️
#security 90.39% <ø> (ø) ⬆️
Impacted Files Coverage Δ
pilot/platform/kube/conversion.go 86.18% <100%> (-0.78%) ⬇️
pilot/model/validation.go 97.1% <100%> (ø) ⬆️
pilot/model/service.go 100% <100%> (ø) ⬆️
pilot/platform/eureka/conversion.go 84.88% <100%> (+0.03%) ⬆️
pilot/platform/consul/conversion.go 89.24% <50%> (+2.21%) ⬆️
pilot/adapter/config/crd/controller.go 77.85% <0%> (-2.02%) ⬇️
pilot/proxy/envoy/ingress.go
pilot/proxy/envoy/infra_auth.go
pilot/proxy/envoy/discovery.go
pilot/proxy/envoy/route.go
... and 7 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 cc39692...5e0495b. Read the comment docs.

Copy link
Copy Markdown
Contributor

@ijsnellf ijsnellf left a comment

Choose a reason for hiding this comment

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

nice refactor! lgtm.

@vadimeisenbergibm
Copy link
Copy Markdown
Contributor Author

@ijsnellf could you please add "lgtm" label? In the Labels section.

@rshriram rshriram merged commit e9a2ce3 into istio:master Nov 27, 2017
vadimeisenbergibm added a commit to vadimeisenbergibm/istio that referenced this pull request Nov 28, 2017
…#1866)

* added ConvertCaseInsensitiveStringToProtocol and ProtocolUnsupported

* remove code duplication in convertProtocol functions

* use ConvertCaseInsensitiveStringToProtocol instead of strings.ToUpper

in conversion of a string to model.Protocol, since there are
not-all-uppercase protocols like Mongo and Redis

* removed unused metadata values

* added TestConvertCaseInsensitiveStringToProtocol

* replaced shadowing variable

* Revert "replaced shadowing variable"

This reverts commit 6a9981f.

* replaced shadowing variable
kyessenov pushed a commit to kyessenov/istio that referenced this pull request Aug 13, 2018
* Reduce log level for jwt filter (istio#1866)

* Update_Dependencies (istio#1873)

* Correctly clean up headers used for payload from JWT authentication (istio#1879)

* Correctly clean up headers used for payload from JWT authentication

* Clang

* Update_Dependencies (istio#1883)

* destination.principal derivation fix (istio#1884)

* fix attribute extraction

Signed-off-by: Kuat Yessenov <kuat@google.com>

* seed mock

Signed-off-by: Kuat Yessenov <kuat@google.com>

* merge 1.0 to master

* Update API SHA (istio#1891)

* add needed dependencies for circle ci
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.

7 participants