Closed ReToCode closed 3 months ago
[APPROVALNOTIFIER] This PR is APPROVED
This pull-request has been approved by: ReToCode
The full list of commands accepted by this bot can be found here.
The pull request process is described here
Built without sensitive environment variables
Name | Link |
---|---|
Latest commit | bd831a116131e3b2c03a825a4f92167ef1439fc6 |
Latest deploy log | https://app.netlify.com/sites/knative/deploys/660bae18b7d6170008a85886 |
Deploy Preview | https://deploy-preview-5855--knative.netlify.app |
Preview on mobile | Toggle QR Code...Use your smartphone camera to open QR code link. |
To edit notification comments on pull requests, go to your Netlify site configuration.
@dprotaso gentle ping.
LGTM
A few suggestions. There is an opportunity to expand the whole section a bit with more info to educate users imho.
It is an opportunity to expand the whole section a bit with more info to educate users imho.
Good point, I'll see what I can add without making it too complex here (e.g. not all internas are really relevant to the "casual" user). But we maybe can also link the FT document that has a whole table of combinations as well?
But we maybe can also link the FT document that has a whole table of combinations as well?
Do we do this elsewhere? I am afraid we might change the implementation without changing the ft doc (happened before) because ft doc has its own lifecycle. So I would say let's keep one place where info is updated and copy what makes sense from the ft doc. wdyth?
I added more details in the docs. But I don't think this is the place to really put all the details about headers, how the components handle those and the exact rewriting. This documentation is focussing on the end-user and not Knative contributors. I think for those folks, the FTs + the actual code should be enough (at least it was for me working on this). An additional documentation is probably just going to be outdated at some point anyway.
Thanks @ReToCode for the updates some I added some minor comments. LGTM, I will wait @dprotaso for his review before merging or if he has no objection I can stamp it.
@skonto updated. I think we can go ahead without Dave here, as we can always update docs later on easily 👍 @dprotaso feel free to veto :)
/retest
/lgtm
Proposed Changes
/assign @dprotaso /assign @skonto