Skip to content

Conversation

@crazy-max
Copy link
Member

@crazy-max crazy-max commented May 21, 2022

URLs of some images are broken for the engine api because of a missing leading slash: https://docs.docker.com/engine/api/v1.41/

image

I took the opportunity with this change to also move the logic in a dedicated plugin that will run in post_read (e.g., will be done after pre_read like the fetch remote resources plugin) and also added ./docker-hub/api/*.yaml pattern which was not covered previously.

We didn't catch that because htmlproofer ignores this folder: https://github.com/docker/docker.github.io/blob/8d6bd6bdb5c1b7b23d196e62760b93b89a588661/Dockerfile#L77

Signed-off-by: CrazyMax crazy-max@users.noreply.github.com

Signed-off-by: CrazyMax <crazy-max@users.noreply.github.com>
@crazy-max crazy-max requested a review from thaJeztah May 21, 2022 06:51
@netlify
Copy link

netlify bot commented May 21, 2022

Deploy Preview for docsdocker ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit 93315b5
🔍 Latest deploy log https://app.netlify.com/sites/docsdocker/deploys/62888c0105e2530007936ddb
😎 Deploy Preview https://deploy-preview-14795--docsdocker.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@thaJeztah
Copy link
Member

Do you know what caused this? Because the original file has a full URL; is one of the replace scripts replacing too much? https://github.com/moby/moby/blob/master/docs/api/v1.41.yaml#L27

@crazy-max
Copy link
Member Author

Removing the exclusion from htmlproofer leads to only one failure:

#15 0.340 
#15 0.340
#15 84.29 Ran on 1708 files!
#15 84.29
#15 84.29
#15 84.29 - ./_site/engine/api/v1.19/index.html
#15 84.29   *  linking to internal hash #bind-docker-to-another-host-port-or-a-unix-socket that does not exist (line 124)
#15 84.29      <a href="https://docs.docker.com/engine/reference/commandline/dockerd/#bind-docker-to-another-host-port-or-a-unix-socket">[Bind Docker to another host/port or a Unix socket](https://docs.docker.com/engine/reference/commandline/dockerd/#bind-docker-to-another-host-port-or-a-unix-socket)</a>
#15 84.29
#15 84.29 HTML-Proofer found 1 failure!

I think we could just exclude ./_site/engine/api/v1.19/index.html.

Dir.glob("./engine/api/*.yaml") do |file_name|
Jekyll.logger.info " #{file_name}"
text = File.read(file_name)
replace = text.gsub!("https://docs.docker.com/", "")
Copy link
Member

Choose a reason for hiding this comment

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

Ah yeah, replace should not strip the /

Are there other places where we do this?

Copy link
Member Author

Choose a reason for hiding this comment

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

I was checking other pages too with hardcoded docs.docker.com which leads to unexpected behavior for review because it doesn't check the actual domain with htmlproofer. I have another branch that will fix that and remove hardcoded root url as you suggested in #14784 (comment)

@crazy-max
Copy link
Member Author

Do you know what caused this? Because the original file has a full URL; is one of the replace scripts replacing too much? https://github.com/moby/moby/blob/master/docs/api/v1.41.yaml#L27

Yes in #14688, when moved to a Jekyll plugin, I forgot to add the leading slash like it was before: https://github.com/docker/docker.github.io/pull/14688/files#diff-4aad9ddf294771293cfaa8016eb98c5a91c56e3f7e45c608df4be2d519a68d2eL38

Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

SGTM

@thaJeztah
Copy link
Member

Just checked the preview on my desktop (looks like mobile view doesn't include an icon), and icon looks good now 👍

@thaJeztah thaJeztah merged commit a64edf0 into docker:master May 21, 2022
@crazy-max crazy-max deleted the fix-swagger-urls branch May 21, 2022 07:26
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.

2 participants