Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Updating Service AppProtocol to GA #24933

Merged
merged 1 commit into from
Nov 21, 2020
Merged

Updating Service AppProtocol to GA #24933

merged 1 commit into from
Nov 21, 2020

Conversation

robscott
Copy link
Member

@robscott robscott commented Nov 6, 2020

This is a PR for changes related to kubernetes/enhancements#1507. I'm not sure what I need to add to update the feature gate related docs though.

/sig network

@k8s-ci-robot k8s-ci-robot added the sig/network Categorizes an issue or PR as relevant to SIG Network. label Nov 6, 2020
@k8sio-netlify-preview-bot
Copy link
Collaborator

k8sio-netlify-preview-bot commented Nov 6, 2020 •
edited
Loading

Deploy preview for kubernetes-io-vnext-staging processing.

Building with commit 9206f25

https://app.netlify.com/sites/kubernetes-io-vnext-staging/deploys/5fb86651f5176500078c0e57

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Nov 6, 2020
@k8s-ci-robot k8s-ci-robot added language/en Issues or PRs related to English language size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. sig/docs Categorizes an issue or PR as relevant to SIG Docs. labels Nov 6, 2020
@robscott robscott mentioned this pull request Nov 6, 2020
Closed
@annajung
Copy link
Contributor

annajung commented Nov 6, 2020 •
edited
Loading

/assign @kcmartin
/milestone 1.20
/hold pending merge kubernetes/kubernetes#96327

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 6, 2020
@k8s-ci-robot k8s-ci-robot added this to the 1.20 milestone Nov 6, 2020
@robscott
Copy link
Member Author

robscott commented Nov 9, 2020

Woops, sorry @annajung! I opened a k/k PR just after that: kubernetes/kubernetes#96327

@annajung
Copy link
Contributor

Hey @robscott, sorry I missed your comment about needing help with docs related to feature gate.

You need to make modification to content/en/docs/reference/command-line-tools-reference/feature-gates.md to add your stable feature under https://github.com/kubernetes/website/blob/master/content/en/docs/reference/command-line-tools-reference/feature-gates.md#feature-gates-for-graduated-or-deprecated-features. When adding your feature into the table, please also move the content from https://github.com/kubernetes/website/blob/master/content/en/docs/reference/command-line-tools-reference/feature-gates.md#feature-gates-for-alpha-or-beta-features into feature-gates-for-graduated-or-deprecated-features, maintaining the history for the alpha and beta in the graduated table.

Thanks!

@annajung
Copy link
Contributor

/hold cancel
k/k pr merged

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 12, 2020
irvifa
irvifa reviewed Nov 14, 2020
View reviewed changes
Copy link
Member

@irvifa irvifa left a comment

Choose a reason for hiding this comment

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

Do we need tech reviewer for this?
/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 14, 2020
sftim
sftim previously requested changes Nov 14, 2020
View reviewed changes
Copy link
Contributor

@sftim sftim left a comment

Choose a reason for hiding this comment

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

https://deploy-preview-24138--kubernetes-io-master-staging.netlify.app/docs/reference/command-line-tools-reference/feature-gates/ still lists ServiceAppProtocol as beta - that needs updating to record the graduation to stable.

/lgtm cancel

content/en/docs/concepts/services-networking/service.md Outdated Show resolved Hide resolved
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 14, 2020
@kcmartin
Copy link
Contributor

Hi @robscott,

A friendly reminder that Docs "Ready for Review" Deadline is this Monday, Nov. 23rd for the 1.20 release.

Please reach out if you have any questions.

@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Nov 20, 2020
@robscott
Copy link
Member Author

@sftim @tengqm Thanks for the feedback! I've moved the feature gate down to the graduated section and updated the docs to include the detail on what values should be set for appProtocol.

@robscott
Copy link
Member Author

@kcmartin Thanks for the reminder! I believe this PR now qualifies as ready for review, let me know if I'm missed anything.

@sftim sftim dismissed their stale review November 20, 2020 08:01

Superseded

sftim
sftim reviewed Nov 20, 2020
View reviewed changes
Copy link
Contributor

@sftim sftim left a comment

Choose a reason for hiding this comment

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

/lgtm

content/en/docs/concepts/services-networking/service.md Outdated
{{< feature-state for_k8s_version="v1.19" state="beta" >}}
The `appProtocol` field provides a way to specify an application protocol for
each Service port. The value of this field is mirrored by corresponding
Endpoints and EndpointSlice resources.
Copy link
Contributor

Choose a reason for hiding this comment

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

(nit)

Suggested change
Endpoints and EndpointSlice resources.
Endpoint and EndpointSlice objects.

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated to use "objects", not as sure about "Endpoint". It looks like we've more consistently used "Endpoints" throughout docs from what I can tell. It's definitely confusing to have a plural resource name, but the alternative of "Endpoint" also feels confusing. Not sure what's best here but for now I've left it as "Endpoints".

Copy link
Contributor

Choose a reason for hiding this comment

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

D'oh. The kind is Endpoints plural isn't it. That was the right call.

Copy link
Member Author

Choose a reason for hiding this comment

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

Haha yeah, it is super confusing, especially alongside the singular EndpointSlice name.

content/en/docs/concepts/services-networking/service.md Show resolved Hide resolved
@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 20, 2020
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: 3b472c8e5d9cf186b2eebeaa1456ee46f24d5b68

@irvifa
Copy link
Member

irvifa commented Nov 20, 2020

Hi @robscott I think once you address the comments we’re good to go

@sftim
Copy link
Contributor

sftim commented Nov 20, 2020

(I'd be happy to merge this as-is, then tweak later)

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 21, 2020
kcmartin
kcmartin approved these changes Nov 21, 2020
View reviewed changes
Copy link
Contributor

@kcmartin kcmartin left a comment

Choose a reason for hiding this comment

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

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 21, 2020
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: 1a8b25c049ae3ee5ffbc95895362570bc27844a8

irvifa
irvifa reviewed Nov 21, 2020
View reviewed changes
Copy link
Member

@irvifa irvifa left a comment

Choose a reason for hiding this comment

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

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: irvifa, kcmartin

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

The pull request process is described here

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 21, 2020
@k8s-ci-robot k8s-ci-robot merged commit 0c31d5b into kubernetes:dev-1.20 Nov 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tengqm tengqm tengqm left review comments

@irvifa irvifa irvifa left review comments

@sftim sftim sftim left review comments

@kcmartin kcmartin kcmartin approved these changes

@zparnold zparnold Awaiting requested review from zparnold

Assignees

@irvifa irvifa

@kcmartin kcmartin

@sftim sftim

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. language/en Issues or PRs related to English language lgtm "Looks good to me", indicates that a PR is ready to be merged. sig/docs Categorizes an issue or PR as relevant to SIG Docs. sig/network Categorizes an issue or PR as relevant to SIG Network. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Milestone
1.20
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@robscott @k8sio-netlify-preview-bot @annajung @kcmartin @k8s-ci-robot @irvifa @sftim @tengqm