-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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.
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
PR for SRVKS-1174: Document how users can utilize full duplex support for http1 #76331
PR for SRVKS-1174: Document how users can utilize full duplex support for http1 #76331
Conversation
@kaldesai: This pull request references SRVKS-1174 which is a valid jira issue. In response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
/label serverless |
@kaldesai: This pull request references SRVKS-1174 which is a valid jira issue. In response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
@kaldesai: This pull request references SRVKS-1174 which is a valid jira issue. In response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
@kaldesai: This pull request references SRVKS-1174 which is a valid jira issue. In response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
/label serverless |
@kaldesai: This pull request references SRVKS-1174 which is a valid jira issue. In response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
@skonto is now ready for review. Can you please take a look and let me know? |
name: example-service | ||
namespace: default | ||
annotations: | ||
features.knative.dev/http-full-duplex: "Enabled" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The annotation should be on the revision template, not Service,
this was fixed upstream in knative/docs#5839 (see the updated example there)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also the docs are here: https://knative.dev/docs/serving/services/http-protocol/#http1-full-duplex-support-per-workload
LGTM in general I would add a pointer to the related Golang issue so people know when to use that. Copying from the upstream docs.
|
/label peer-review-needed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you end up changing the topic_map per suggestion, please re-run this through the peer-review queue. Also reminder to squash (and remove WIP from PR title) before merge-review. Thanks!
|
||
You can enable the HTTP/1 full duplex support for a service by configuring the `features.knative.dev/http-full-duplex` annotation. | ||
|
||
NOTE: Verify your HTTP clients before enabling, as older clients may not provide support for HTTP/1 full duplex. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Style guide for admonitions
- [ISG] Use "earlier" for "older"
- [ISG] Use "might" for "may"
NOTE: Verify your HTTP clients before enabling, as older clients may not provide support for HTTP/1 full duplex. | |
[NOTE] | |
==== | |
Verify your HTTP clients before enabling, as earlier version clients might not provide support for HTTP/1 full duplex. | |
==== |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
- Name: HTTP configuration | ||
Dir: http-configuration | ||
Topics: | ||
- Name: Global HTTPS redirection | ||
File: https-redirect-global | ||
- Name: HTTPS redirection per service | ||
File: https-redirect-per-service | ||
- Name: Full duplex support for HTTP/1 | ||
File: http1-full-duplex-support |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this need to be a full subdirectory? Especially considering each of the 3 assemblies only have 1 module in each (with no TOCs).
Could this instead just be a single new "HTTP configuration" assembly that include::
s the 3 modules (the 2 old ones and the 1 new one)? I feel like that would still satisfy the Jira ticket's request to "create another section".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it does need to be a full subdirectory as each assemblies are going to expand by the upcoming next release and will contain the multiple modules. That is why I created like that. :)
41c165c
to
56d11dd
Compare
@kaldesai: all tests passed! Full PR test history. Your PR dashboard. 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-sigs/prow repository. I understand the commands that are listed here. |
/label merge-review-needed |
@kaldesai I think this PR is OK to merge. When you are ready, remove the do-not-merge label and let us know. |
@mburke5678 I have removed the do-not-merge label, you can go ahead and merge this. Thank you! :) |
1 similar comment
@mburke5678 I have removed the do-not-merge label, you can go ahead and merge this. Thank you! :) |
/cherrypick serverless-docs-1.33 |
/cherrypick serverless-docs-1.32 |
@mburke5678: new pull request created: #76865 In response to this:
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-sigs/prow repository. |
@mburke5678: new pull request created: #76866 In response to this:
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-sigs/prow repository. |
Affected versions for cherry-picking:
serverless-docs-1.33
serverless-docs-1.32
Tracking JIRA: https://issues.redhat.com/browse/SRVKS-1174
Doc preview:
I have created an HTTP configuration section, which now includes: