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

Fix broken links in lists and indented text #248

Merged
merged 1 commit into from
Feb 19, 2021

Conversation

afrittoli
Copy link
Member

Changes

When replacing links, we parse the markdown document line by line. When
the markdown parsers encounters an indented line, without the context of
the surrounding lines, it assumes a code block and does not render the
links to html, which means we do not re-write those links.

This patch fixes the issue by stripping left spaces in the text
fragments sent to "get_links", so that we can extract links successfully
and preserve the spaces in the rendered markdown.

Fixes: #229

Signed-off-by: Andrea Frittoli andrea.frittoli@gmail.com

Submitter Checklist

These are the criteria that every PR should meet, please check them off as you
review them:

See the contribution guide
for more details.

/kind bug
/cc @AlanGreene

@tekton-robot tekton-robot added kind/bug Categorizes issue or PR as related to a bug. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Feb 19, 2021
@AlanGreene
Copy link
Member

AlanGreene commented Feb 19, 2021

The link still appears to be broken:

https://deploy-preview-248--tekton.netlify.app/docs/pipelines/pipelines/pipelineruns.md#service-accounts

should be:

https://deploy-preview-248--tekton.netlify.app/docs/pipelines/pipelineruns#service-accounts

Also the #service-accounts fragment doesn't exist in the pipelineruns doc, I'm guessing it's intended to link to #specifying-custom-serviceaccount-credentials.

Edit: Ah I see there were 2 issues reported, this PR fixes the issue with the Interceptors link on the Triggers page 👍 The other issue remains.

@afrittoli
Copy link
Member Author

The link still appears to be broken:

https://deploy-preview-248--tekton.netlify.app/docs/pipelines/pipelines/pipelineruns.md#service-accounts

should be:

https://deploy-preview-248--tekton.netlify.app/docs/pipelines/pipelineruns#service-accounts

Also the #service-accounts fragment doesn't exist in the pipelineruns doc, I'm guessing it's intended to link to #specifying-custom-serviceaccount-credentials.

Edit: Ah I see there were 2 issues reported, this PR fixes the issue with the Interceptors link on the Triggers page 👍 The other issue remains.

Indeed, the other issue is that the original text breaks the link on two lines:

See the [Service Account](
pipelineruns.md#service-accounts)

Since that is supported by the HTML renderer, we should probably support it too... but to support that we'll have to change the behaviour of the script a bit, the line by line parsing won't be able to detect the link in there.

@AlanGreene
Copy link
Member

I think it's reasonable to expect that links wouldn't be split across multiple lines like that in the markdown. While technically we could update the parser to detect this type of case I'm not sure it's worth the added complexity and we're likely to be missing other cases too. I'd be happy just to fix the markdown. What do you think?

@afrittoli
Copy link
Member Author

That's true, however I'm afraid there may be more errors hidden in the current line-by-line parsing approach.
I think we could just switch to work on the document as a whole.

@tekton-robot tekton-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Feb 19, 2021
When replacing links, we parse the markdown document line by line. When
the markdown parsers encounters an indented line, without the context of
the surrounding lines, it assumes a code block and does not render the
links to html, which means we do not re-write those links.

If the link is broken over two lines we also fail to discover it.
This patch fixes the issue by changing the processing from line by line
to the document as a whole.

Fixes: tektoncd#229

Signed-off-by: Andrea Frittoli <andrea.frittoli@gmail.com>
@tekton-robot tekton-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Feb 19, 2021
@afrittoli
Copy link
Member Author

@tektoncd/website-maintainers All broken links reported in #229 are fixed by this, except for an invalid fragment which is fixed in tektoncd/pipeline#3773.
It would be good to merge this so we get rid of several broken links.

Copy link
Member

@AlanGreene AlanGreene left a comment

Choose a reason for hiding this comment

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

Looks good. Thanks for adding the extra tests too 👍

/lgtm

@tekton-robot tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 19, 2021
@tekton-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: AlanGreene

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

@tekton-robot tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 19, 2021
@tekton-robot tekton-robot merged commit b67a9e3 into tektoncd:main Feb 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. kind/bug Categorizes issue or PR as related to a bug. lgtm Indicates that a PR is ready to be merged. 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.

Pipelines
3 participants