Skip to content

Add annotations to services defined in Charts #5760

Merged
rshriram merged 1 commit intoistio:masterfrom
etiennetremel:add-annotations-to-services-release-0.8
Jun 20, 2018
Merged

Add annotations to services defined in Charts #5760
rshriram merged 1 commit intoistio:masterfrom
etiennetremel:add-annotations-to-services-release-0.8

Conversation

@etiennetremel
Copy link
Copy Markdown
Contributor

@etiennetremel etiennetremel commented May 22, 2018 •
edited
Loading

  • add annotations to services defined in Charts
  • make use service type from values.yaml for tracing services

@etiennetremel etiennetremel mentioned this pull request May 22, 2018
Closed
@rshriram
Copy link
Copy Markdown
Member

rshriram commented May 23, 2018

/ok-to-test

@costinm
Copy link
Copy Markdown
Contributor

costinm commented May 24, 2018

Jaeger is getting reverted - so you'll likely get some conflicts.

Let's move it to master if possible, I don't think it's release blocking.

@etiennetremel etiennetremel force-pushed the add-annotations-to-services-release-0.8 branch from 6507604 to e79c3ad Compare May 24, 2018 07:44
@codecov
Copy link
Copy Markdown

codecov bot commented May 24, 2018 •
edited
Loading

Codecov Report

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

Impacted file tree graph

@@           Coverage Diff           @@
##           master   #5760    +/-   ##
=======================================
- Coverage      68%     68%   -<1%     
=======================================
  Files         351     351            
  Lines       30855   30691   -164     
=======================================
- Hits        20826   20688   -138     
+ Misses       9179    9163    -16     
+ Partials      850     840    -10
Impacted Files Coverage Δ
mixer/adapter/solarwinds/log_handler.go 50% <0%> (-30%) ⬇️
istioctl/pkg/writer/pilot/status.go 84% <0%> (-7%) ⬇️
...olarwinds/internal/papertrail/papertrail_logger.go 57% <0%> (-4%) ⬇️
pilot/pkg/proxy/envoy/v2/debug.go 27% <0%> (-2%) ⬇️
pilot/pkg/proxy/envoy/v2/ads.go 71% <0%> (-2%) ⬇️
pilot/pkg/serviceregistry/consul/controller.go 71% <0%> (-2%) ⬇️
...t/pkg/serviceregistry/external/servicediscovery.go 80% <0%> (-1%) ⬇️
...lot/pkg/serviceregistry/eureka/servicediscovery.go 88% <0%> (-1%) ⬇️
mixer/adapter/bypass/util.go 7% <0%> (ø) ⬇️
mixer/adapter/cloudwatch/handler.go 87% <0%> (ø) ⬇️
... and 22 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 e3ab338...ed8df2d. Read the comment docs.

@etiennetremel
Copy link
Copy Markdown
Contributor Author

etiennetremel commented May 24, 2018

That's unfortunate, I rebased the branch. This is definitely not a blocker but nice to have. Currently I have a workaround to patch some of the services because of the missing annotations field.

@etiennetremel etiennetremel force-pushed the add-annotations-to-services-release-0.8 branch from e79c3ad to d3c1b43 Compare May 25, 2018 07:42
@etiennetremel
Copy link
Copy Markdown
Contributor Author

etiennetremel commented May 25, 2018

Fixed conflicts since Jaeger is back again (#5840)

@etiennetremel etiennetremel mentioned this pull request May 25, 2018
Merged
objectiser
objectiser reviewed May 25, 2018
View reviewed changes
install/kubernetes/helm/istio/charts/tracing/templates/service-jaeger.yaml Outdated
Copy link
Copy Markdown
Contributor

@objectiser objectiser May 25, 2018

Choose a reason for hiding this comment

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

This needs to be defined as a different type to the collector service - so can you create a different property in the values.yaml (e.g. .Values.service.uiType) which should be set to LoadBalancer?

Copy link
Copy Markdown
Contributor Author

@etiennetremel etiennetremel May 25, 2018

Choose a reason for hiding this comment

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

in fact, much better. I just made the change. Thanks

objectiser
objectiser reviewed May 25, 2018
View reviewed changes
install/kubernetes/helm/istio/charts/tracing/templates/service.yaml Outdated
Copy link
Copy Markdown
Contributor

@objectiser objectiser May 25, 2018

Choose a reason for hiding this comment

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

Same as above, should use a different property, e.g. .Values.service.uiType set to LoadBalancer.

@etiennetremel etiennetremel changed the base branch from release-0.8 to master June 6, 2018 20:22
@etiennetremel etiennetremel changed the title [release 0.8] Add annotations to services defined in Charts Add annotations to services defined in Charts Jun 6, 2018
@istio-testing istio-testing added the needs-rebase Indicates a PR needs to be rebased before being merged label Jun 6, 2018
@etiennetremel etiennetremel force-pushed the add-annotations-to-services-release-0.8 branch from b2482f9 to 1110c7c Compare June 7, 2018 08:20
@istio-testing istio-testing removed the needs-rebase Indicates a PR needs to be rebased before being merged label Jun 7, 2018
@istio-testing istio-testing added the needs-rebase Indicates a PR needs to be rebased before being merged label Jun 17, 2018
gyliu513
gyliu513 reviewed Jun 18, 2018
View reviewed changes
install/kubernetes/helm/istio/charts/tracing/templates/service.yaml Outdated
Copy link
Copy Markdown
Member

@gyliu513 gyliu513 Jun 18, 2018

Choose a reason for hiding this comment

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

We do not want to expose service type here, and intend to use Gateway to expose those services.

Suggest your PR focus on adding annotations but do not touch service type in this PR

Copy link
Copy Markdown
Contributor Author

@etiennetremel etiennetremel Jun 20, 2018

Choose a reason for hiding this comment

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

Just removed any changes that are unrelated to the annotations.

@etiennetremel etiennetremel force-pushed the add-annotations-to-services-release-0.8 branch from 1110c7c to ce35634 Compare June 20, 2018 07:44
@istio-testing
Copy link
Copy Markdown
Collaborator

istio-testing commented Jun 20, 2018

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: etiennetremel
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: ostromart

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

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

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@istio-testing
Copy link
Copy Markdown
Collaborator

istio-testing commented Jun 20, 2018 •
edited
Loading

@etiennetremel: The following tests failed, say /retest to rerun them all:

Test name Commit Details Rerun command
prow/istio-pilot-e2e.sh 1110c7c0d0b87bba65f14b9b63c09c27d6f73e63 link /test istio-pilot-e2e
prow/e2e-bookInfoTests.sh 1110c7c0d0b87bba65f14b9b63c09c27d6f73e63 link /test e2e-bookInfo
Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

gyliu513
gyliu513 reviewed Jun 20, 2018
View reviewed changes
Copy link
Copy Markdown
Member

@gyliu513 gyliu513 left a comment

Choose a reason for hiding this comment

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

@etiennetremel please rebase

@etiennetremel etiennetremel force-pushed the add-annotations-to-services-release-0.8 branch from ce35634 to ed8df2d Compare June 20, 2018 08:26
@istio-testing istio-testing removed the needs-rebase Indicates a PR needs to be rebased before being merged label Jun 20, 2018
@rshriram rshriram merged commit b950c88 into istio:master Jun 20, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

3 more reviewers

@objectiser objectiser objectiser left review comments

@gyliu513 gyliu513 gyliu513 left review comments

@rshriram rshriram rshriram approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@etiennetremel @rshriram @costinm @istio-testing @objectiser @gyliu513 @googlebot