Skip to content
This repository has been archived by the owner on Oct 7, 2020. It is now read-only.

add service to k8s spec #711

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

add service to k8s spec #711

wants to merge 5 commits into from

Conversation

richardwxn
Copy link
Contributor

@richardwxn richardwxn commented Dec 20, 2019 •
edited
Loading

the service part was added to k8s proto under ISCP spec before. however it is not added to tranlationConfig so it is not doing anything if we add the settings under component.k8s.service

@richardwxn richardwxn requested a review from a team as a code owner December 20, 2019 02:01
@googlebot googlebot added the cla: yes Set by the Google CLA bot to indicate the author of a PR has signed the Google CLA. label Dec 20, 2019
@istio-testing istio-testing added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Dec 20, 2019
ostromart
ostromart reviewed Dec 20, 2019
View reviewed changes
cmd/mesh/testdata/manifest-generate/input/ingressgateway_k8s_settings.yaml Outdated
enabled: true
k8s:
service:
externalTrafficPolicy: Local
Copy link
Contributor

Choose a reason for hiding this comment

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

Some more complex example would be good. For example, adding to list of ports, which I'm actually not sure is going to work with this change...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ostromart I added the mirror of k8s service type so that unmarshalling can work with serviceport. PTAL

Copy link
Contributor

Choose a reason for hiding this comment

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

lgtm, but could you hold off merging till #713 goes in? it will be easier to address any merge changes here rather than there.

@istio-testing istio-testing added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Dec 20, 2019
This was referenced Dec 22, 2019
Closed
Closed
elfinhe
elfinhe reviewed Dec 30, 2019
View reviewed changes
Copy link
Member

@elfinhe elfinhe left a comment

Choose a reason for hiding this comment

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

LGTM if the test mentioned by Martin is added.

@ostromart ostromart added the do-not-merge Block automatic merging of a PR. label Jan 9, 2020
@richardwxn
Copy link
Contributor Author

need to wait for the change in api repo: istio/api#1229

@richardwxn richardwxn mentioned this pull request Jan 10, 2020
Merged
@istio-testing
Copy link

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

Test name Commit Details Rerun command
test_operator c829065 link /test test_operator
e2e_operator c829065 link /test e2e_operator

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.

@ostromart
Copy link
Contributor

@richardwxn please move this pr to istio/istio.

@richardwxn richardwxn mentioned this pull request Jan 16, 2020
Merged
@istio-testing istio-testing added the needs-rebase Indicates a PR needs to be rebased before being merged label Feb 12, 2020
@istio-testing
Copy link

@richardwxn: PR needs rebase.

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.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@ostromart ostromart Awaiting requested review from ostromart

@elfinhe elfinhe Awaiting requested review from elfinhe

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
cla: yes Set by the Google CLA bot to indicate the author of a PR has signed the Google CLA. do-not-merge Block automatic merging of a PR. needs-rebase Indicates a PR needs to be rebased before being merged size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@richardwxn @istio-testing @ostromart @elfinhe @googlebot