Skip to content

Conversation

@crazy-max
Copy link
Member

@crazy-max crazy-max commented Jul 19, 2022

needs moby/buildkit#2970

as discussed, it's better to have the frontmatter section here instead of the buildkit repo. needs to fetch remote reference in the same folder otherwise we can't include it in the stub file. will be fixed anyway when we are going to move to the new location in #15110

@crazy-max crazy-max requested a review from thaJeztah July 19, 2022 18:08
@netlify
Copy link

netlify bot commented Jul 19, 2022

Deploy Preview for docsdocker ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit 57144bd
🔍 Latest deploy log https://app.netlify.com/sites/docsdocker/deploys/62de556c61744600083aeeac
😎 Deploy Preview https://deploy-preview-15165--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.

@crazy-max
Copy link
Member Author

crazy-max commented Jul 20, 2022

actually not so great atm as we have to fetch remote reference in the same folder so we can include it with liquid template func include or include_relative. problem is this file could be discovered by the end user. keeping in draft for now, we need to find a better way.

@crazy-max crazy-max force-pushed the dockerfile-ref-frontmatter branch from 8534df5 to c86c00e Compare July 20, 2022 05:45
@crazy-max crazy-max force-pushed the dockerfile-ref-frontmatter branch from c86c00e to 38622ff Compare July 20, 2022 05:55
@crazy-max crazy-max mentioned this pull request Jul 20, 2022
_config.yml Outdated
Comment on lines 51 to 52
repo: crazy-max/buildkit
ref: docs-ref-move-frontmatter
Copy link
Member Author

Choose a reason for hiding this comment

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

use my fork for testing

@thaJeztah
Copy link
Member

actually not so great atm as we have to fetch remote reference in the same folder so we can include it with liquid template func include or include_relative.

I'm a bit confused though; for other includes, we use the _includes directory (and subdirectories thereof) can't we do the same for this?

@thaJeztah
Copy link
Member

Or is that because of relative links to markdown files?

@crazy-max
Copy link
Member Author

crazy-max commented Jul 20, 2022

I'm a bit confused though; for other includes, we use the _includes directory (and subdirectories thereof) can't we do the same for this?

Hum indeed it might work with the _includes folder as it's not rendered 👍

@crazy-max crazy-max force-pushed the dockerfile-ref-frontmatter branch from 38622ff to 9262650 Compare July 20, 2022 09:39
_config.yml Outdated
Comment on lines 184 to 186
- file: "frontend/dockerfile/docs/reference.md"
line_start: 2
line_end: -1
Copy link
Member Author

Choose a reason for hiding this comment

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

@thaJeztah As discussed, removed the github_content liquid tag plugin and enhance our fetch_remote plugin to be able to specify line_start and line_end

Copy link
Member Author

Choose a reason for hiding this comment

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

Argh edit_url and issue_url is lost then 😣

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok managed to fix edit_url and issue_url inclusion by grabbing the one available in site default scope. I have created a dedicated liquid tag named include_remote that does pretty much the same job as include except it also handles <line_start>, <line_end> and creates the right edit_url and issue_url.

LGTY @thaJeztah ?

Signed-off-by: CrazyMax <crazy-max@users.noreply.github.com>
Signed-off-by: CrazyMax <crazy-max@users.noreply.github.com>
@crazy-max crazy-max force-pushed the dockerfile-ref-frontmatter branch from 9262650 to 04ae02b Compare July 20, 2022 10:48
_config.yml Outdated
Comment on lines 178 to 180
- repo: "https://github.com/crazy-max/buildkit"
default_branch: "master"
ref: "master"
ref: "docs-ref-move-frontmatter"
Copy link
Member Author

Choose a reason for hiding this comment

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

use my fork while waiting for moby/buildkit#2970 to be merged

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.

LGTM after switching to upstream once moby/buildkit#2970 is merged

@crazy-max crazy-max force-pushed the dockerfile-ref-frontmatter branch from 04ae02b to 57144bd Compare July 25, 2022 08:33
@crazy-max crazy-max marked this pull request as ready for review July 25, 2022 08:33
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.

LGTM

@thaJeztah thaJeztah merged commit dd23e51 into docker:master Jul 25, 2022
@crazy-max crazy-max deleted the dockerfile-ref-frontmatter branch July 25, 2022 08:43
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