Skip to content

Conversation

@crazy-max
Copy link
Member

follow-up #15179 and #15250

Some links are broken in our lab environment because of absolute URLs being used and the DOCS_URL env var not being taken into account properly.

This PR removes most of the absolute URLs and use local .md files when available, otherwise a relative URL.

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

@netlify
Copy link

netlify bot commented Jul 31, 2022

Deploy Preview for docsdocker ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit 09c9f51
🔍 Latest deploy log https://app.netlify.com/sites/docsdocker/deploys/62ea44f10af0a20008b860ee
😎 Deploy Preview https://deploy-preview-15276--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.

1.6 exactly as they do today.

Check the upgrade guide for full details:
https://docs.docker.com/compose/compose-file#upgrading
Copy link
Member Author

Choose a reason for hiding this comment

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

This URL was not valid. We could not catch it with htmlproofer because not solved as link.

Copy link
Contributor

@dockertopia dockertopia Aug 2, 2022

Choose a reason for hiding this comment

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

That's a line in the file from 2016, probably there were changes in compose-file affecting this.
You say "DOCS_URL env var not being taken into account properly.", how should we take this var into account?
I'm unaware of this topic. cc: @usha-mandya

Copy link
Member Author

Choose a reason for hiding this comment

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

You say "DOCS_URL env var not being taken into account properly.", how should we take this var into account?
I'm unaware of this topic. cc: @usha-mandya

DOCS_URL has been added in #15250 to avoid hardcoded URLs that would not match the environment (stage vs lab vs prod). See for example the source of https://docs-stage.docker.com/ and you will see some URLs with docs.docker.com which is not right and should be docs-stage.docker.com.

tldr; this is an internal enhancement that should not impact your workflow.

Copy link
Contributor

Choose a reason for hiding this comment

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

Got it, so to get right base URL according with environment. Thanks for the explanation.

@crazy-max crazy-max force-pushed the rm-absolute-links branch 2 times, most recently from 041095e to b743f93 Compare August 2, 2022 16:41
**Bug fixes and minor changes**

* Documentation moved to [https://docs.docker.com/desktop/mac/](../index.md)
* Documentation moved to [{{ site.docs_url }}/desktop/mac/](../index.md)
Copy link
Member

Choose a reason for hiding this comment

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

Do we need {{ site.docs_url }} here? Wondering if it should be the same as other relative URLs on this page?

Copy link
Member Author

@crazy-max crazy-max Aug 3, 2022

Choose a reason for hiding this comment

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

I think here it's implicitly showing the absolute url but agree that /desktop/mac/ is enough, will fix that.

@usha-mandya
Copy link
Member

@crazy-max Added a minor comment on one of the Desktop pages. Looks good otherwise. Thank you!

Signed-off-by: CrazyMax <crazy-max@users.noreply.github.com>
@crazy-max
Copy link
Member Author

@usha-mandya Should be good, thanks for the review

Copy link
Member

@usha-mandya usha-mandya left a comment

Choose a reason for hiding this comment

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

LGTM. Thank you

@crazy-max crazy-max merged commit 02d195d into docker:master Aug 3, 2022
@crazy-max crazy-max deleted the rm-absolute-links branch August 3, 2022 10:30
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