Skip to content

Fixed absolute links in external content#886

Merged
johndmulhausen merged 1 commit into
docker:masterfrom
aduermael:fix-links-in-external-content
Dec 16, 2016
Merged

Fixed absolute links in external content#886
johndmulhausen merged 1 commit into
docker:masterfrom
aduermael:fix-links-in-external-content

Conversation

@aduermael

Copy link
Copy Markdown
Contributor

Proposed changes

The files we’re pulling from docker/docker may include links to docs.docker.com. And we can’t forbid that because relative links wouldn’t make sense in the context of docker/docker repository in some situations. So let’s just fix these links right after they get imported.

This is necessary for tests in #849 to pass.

The files we’re pulling from docker/docker may include links to docs.docker.com. And we can’t forbid that because relative links wouldn’t make sense in the context of docker/docker repository in some situations. So let’s just fix these links right after get imported.

Signed-off-by: Adrien Duermael <adrien@duermael.com>
@aduermael

Copy link
Copy Markdown
Contributor Author

ping @johndmulhausen

Comment thread Dockerfile
&& wget -O allv/registry/configuration.md https://raw.githubusercontent.com/docker/distribution/$DISTRIBUTION_BRANCH/docs/configuration.md \
&& jekyll build -s allv -d allvbuild \
&& find allvbuild/engine/reference -type f -name '*.html' -print0 | xargs -0 sed -i 's#href="https://docs.docker.com/#href="/#g' \
&& find allvbuild/engine/extend -type f -name '*.html' -print0 | xargs -0 sed -i 's#href="https://docs.docker.com/#href="/#g' \

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Adding the obligatory link to the "don't use regex to parse HTML" post http://stackoverflow.com/a/1732454/1811501 here 😇

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

But yeah, I guess this works; when thinking about this, I wondered if we could simply strip https://docs.docker.com/ in case there's links somehow that used single quotes, but I assume that's not a problem here

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

hehe, yes, I don't like doing this too much...

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nobody likes doing this - it's just the worst. :/

@aduermael

Copy link
Copy Markdown
Contributor Author

@johndmulhausen can you merge that one please if you don't have comments?

These links are the only reason why this new test fails: #849

18:55:52 --- FAIL: TestURLs (2.11s)
18:55:52 	html_test.go:53: found absolute link: <a href="https://docs.docker.com/engine/admin/logging/overview/"> - /engine/reference/commandline/logs/index.html
18:55:52 	html_test.go:53: found absolute link: <a href="https://docs.docker.com/engine/tutorials/dockervolumes/"> - /engine/reference/commandline/run/index.html
18:55:52 	html_test.go:53: found absolute link: <a href="https://docs.docker.com/engine/swarm/ingress/"> - /engine/reference/commandline/service_create/index.html

I would like to move forward and keep fixing content in docker/docker.github.io. If we don't need these 2 lines in a near future, we can simply remove them.

Thanks!

@thaJeztah thaJeztah left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

@aduermael

Copy link
Copy Markdown
Contributor Author

thanks @thaJeztah !

@johndmulhausen

Copy link
Copy Markdown
Contributor

Thanks, @aduermael :)

@johndmulhausen johndmulhausen merged commit 609c667 into docker:master Dec 16, 2016
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.

3 participants