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.

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

backlinks from example manifests to guides #1083

Closed
wants to merge 1 commit into from

Conversation

kundan2707
Copy link
Contributor

/kind documentation

What this PR does / why we need it:

Which issue(s) this PR fixes:

Fixes #446

Does this PR introduce a user-facing change?:

None

@k8s-ci-robot k8s-ci-robot added kind/documentation Categorizes issue or PR as related to documentation. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Mar 31, 2022
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: kundan2707
To complete the pull request process, please assign dcbw after the PR has been reviewed.
You can assign the PR to them by writing /assign @dcbw in a comment when ready.

The full list of commands accepted by this bot can be found 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

@youngnick
Copy link
Contributor

Nice work!

/lgtm but will leave for someone else to hit the approve button.

@robscott
Copy link
Member

robscott commented Apr 1, 2022

Thanks for the work on this @kundan2707! This looks great, but unfortunately it had some unintended consequences :(. All of these comments end up being included in the docs as well: https://deploy-preview-1083--kubernetes-sigs-gateway-api.netlify.app/v1alpha2/guides/traffic-splitting/#guide. I spent a bit of time trying to figure out if there was a workaround here that would allow us to exclude comments from the rendered docs, but couldn't find a great solution for this.

Looking back at the original issue, it seems like the goal was to prevent code snippets from being updated without first considering all the files that would be affected by the change. Maybe a better solution here would be to have some kind of GitHub action/presubmit that did the following:

  1. Check to see if any examples had been modified
  2. Search to see if those examples have been included in any markdown
  3. If 1 and 2 are true, add a comment on the PR with a list of examples that have been changed + where they are referenced in docs

I realize that's significantly more complicated than this approach, so open to alternatives. Unfortunately I think having these comments visible in our docs would be too distracting, so I want to put a hold on this until we can find a way around that or build some kind of automation like I suggested above.

/hold

@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 Apr 1, 2022
@hbagdi
Copy link
Contributor

hbagdi commented Apr 1, 2022

Great work.
Rob's concern is fair.
Another path would be to play around with the plugin to see if we can exclude some (not all maybe) comments from the YAML.
For example: https://pypi.org/project/mkdocs-include-markdown-plugin/
This would require a bit more work on the tooling side.

@kundan2707
Copy link
Contributor Author

Thanks for the work on this @kundan2707! This looks great, but unfortunately it had some unintended consequences :(. All of these comments end up being included in the docs as well: https://deploy-preview-1083--kubernetes-sigs-gateway-api.netlify.app/v1alpha2/guides/traffic-splitting/#guide. I spent a bit of time trying to figure out if there was a workaround here that would allow us to exclude comments from the rendered docs, but couldn't find a great solution for this.

Looking back at the original issue, it seems like the goal was to prevent code snippets from being updated without first considering all the files that would be affected by the change. Maybe a better solution here would be to have some kind of GitHub action/presubmit that did the following:

  1. Check to see if any examples had been modified
  2. Search to see if those examples have been included in any markdown
  3. If 1 and 2 are true, add a comment on the PR with a list of examples that have been changed + where they are referenced in docs

I realize that's significantly more complicated than this approach, so open to alternatives. Unfortunately I think having these comments visible in our docs would be too distracting, so I want to put a hold on this until we can find a way around that or build some kind of automation like I suggested above.

/hold

@robscott Thanks for you suggestion. I will look for an alternative and discuss once found.

@k8s-ci-robot
Copy link
Contributor

@kundan2707: 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.

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 6, 2022
@youngnick
Copy link
Contributor

I think that something like this is a good idea, but we need a way to not have the links show up in the rendered site version. I'm going to close this out for now, and encourage @kundan2707 to open a new issue to talk about the approach first.

Thanks for the work @kundan2707, hopefully we can work together to figure out the right way to proceed.

/close

@k8s-ci-robot
Copy link
Contributor

@youngnick: Closed this PR.

In response to this:

I think that something like this is a good idea, but we need a way to not have the links show up in the rendered site version. I'm going to close this out for now, and encourage @kundan2707 to open a new issue to talk about the approach first.

Thanks for the work @kundan2707, hopefully we can work together to figure out the right way to proceed.

/close

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 join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. kind/documentation Categorizes issue or PR as related to documentation. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add backlinks from example manifests to guides
5 participants