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

Docs: Fix broken links on generated site #2600

Merged
merged 1 commit into from
Sep 4, 2020
Merged

Conversation

slim-bean
Copy link
Collaborator

No description provided.

@oddlittlebird
Copy link
Contributor

In order to work in the build, all links need to be relref links. Format is: [Link text]({{< relref "www.example.com" >}}).

@slim-bean
Copy link
Collaborator Author

In order to work in the build, all links need to be relref links. Format is: [Link text]({{< relref "www.example.com" >}}).

Which are you talking about? The external links?

@codecov-commenter
Copy link

Codecov Report

Merging #2600 into master will decrease coverage by 0.00%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2600      +/-   ##
==========================================
- Coverage   62.95%   62.95%   -0.01%     
==========================================
  Files         169      169              
  Lines       15018    15018              
==========================================
- Hits         9455     9454       -1     
+ Misses       4809     4807       -2     
- Partials      754      757       +3     
Impacted Files Coverage Δ
pkg/promtail/targets/file/tailer.go 70.83% <0.00%> (-4.17%) ⬇️
pkg/promtail/targets/file/filetarget.go 66.85% <0.00%> (+0.57%) ⬆️
pkg/querier/queryrange/downstreamer.go 97.93% <0.00%> (+2.06%) ⬆️

@oddlittlebird
Copy link
Contributor

oddlittlebird commented Sep 4, 2020 via email

@slim-bean
Copy link
Collaborator Author

That seems untenable and unnecessary? The links definitely work as is. Asking users to understand the bizarre hugo syntax for this seems unnecessary to me? Especially when they work without it.

Are there other reasons this would be helpful or necessary I'm not aware of?

@oddlittlebird
Copy link
Contributor

That is a @robbymilo question

@slim-bean
Copy link
Collaborator Author

Ok, I will follow up with Robby!

I would like to merge this as is however because all these links are currently broken and this will unbreak them.

If we need to go through and replace all our internal links with relref style I think that might be best done in a separate PR where we can sweep the entire site.

@slim-bean
Copy link
Collaborator Author

I looked at the docs for relref and it looks like it would allow writing the links to the exact file names with the .md extension, I can see some benefits here for sure.

My concern would be how difficult this makes adding new docs unless you are familiar with Hugo, that being said it's still going to be easy to get them wrong as non relref links if you accidentally link to the markdown.

Ultimately I want to get the htmltest app I used to find all these into our CI so either way broken links are caught.

I'm also wondering if there is a hugo linter we could use to automate suggesting of the relref link style whenever someone links to a .md file

Copy link
Member

@owen-d owen-d left a comment

Choose a reason for hiding this comment

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

lgtm - I'm not weighing in on the relref bits but we can get these fixed in the meantime.

@slim-bean slim-bean merged commit 1664a98 into master Sep 4, 2020
@slim-bean slim-bean deleted the fix-broken-links branch September 4, 2020 17:33
@robbymilo
Copy link
Contributor

In order to work in the build, all links need to be relref links. Format is: [Link text]({{< relref "www.example.com" >}}).

This is not required for the build to work, just a nice to have. @oddlittlebird @slim-bean

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants