Skip to content

Conversation

@crazy-max
Copy link
Member

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

This is a follow-up of #14644 (comment) to start working with the native netlify client to leverage the possibility to fetch remote resources.

This PR is just a prep to handle that because the current Dockerfile use a quite old jekyll image starefossen/github-pages:198 (~3 years old) that doesn't support some Gem dependencies.

Instead of relying on a pre-built Jekyll image I switched to a vanilla ruby image.

I also vendor and push the Gemfile.lock in the repo as it should so we can pin to a specific Jekyll version as well as the plugins being used in our configuration. Atm it's Jekyll 4.2.2. I have created a dedicated stage to vendor our Gemfile in case we want to update and vendor the lock file ourselves:

$ make vendor
# or
$ docker buildx bake vendor

This is important so we are sync with our lock file while installing dependencies and deploying. Otherwise we could have unexpected behaviors if Jekyll or its plugins are updated. Same spirit as the go.sum.

I have also updated the GHA workflows accordingly. Also merge our build-*.yml altogether as there are some dedup logic that can be mutualized. Let me know what you think about this one.

args = {
JEKYLL_ENV = JEKYLL_ENV
}
no-cache-filter = ["upstream-resources"]
Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks to the no-cache-filter there is no need to use docker build --no-cache. Just this specific stage will be invalidated.

@crazy-max crazy-max force-pushed the enhanced-dockerfile branch 3 times, most recently from 26543e2 to 2c659c0 Compare May 6, 2022 18:55
@netlify
Copy link

netlify bot commented May 6, 2022

Deploy Preview for docsdocker ready!

Name Link
🔨 Latest commit 26543e2
🔍 Latest deploy log https://app.netlify.com/sites/docsdocker/deploys/62756ee5e434700008f91fec
😎 Deploy Preview https://deploy-preview-14685--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 May 6, 2022

image

required checks should switch to the new ones if we merge this PR as the workflow is now mutualized to avoid dedup.

@netlify
Copy link

netlify bot commented May 6, 2022

Deploy Preview for docsdocker ready!

Name Link
🔨 Latest commit 0f493a1
🔍 Latest deploy log https://app.netlify.com/sites/docsdocker/deploys/627ebaa93295a7000817efef
😎 Deploy Preview https://deploy-preview-14685--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 crazy-max force-pushed the enhanced-dockerfile branch from 2c659c0 to bddf9b5 Compare May 6, 2022 19:17
Comment on lines 104 to 102
FROM gem AS htmlproofer
RUN --mount=type=bind,from=generate,source=/out,target=_site \
--mount=type=cache,target=/usr/local/bundle \
htmlproofer ./_site \
--disable-external \
--internal-domains="docs.docker.com,docs-stage.docker.com,localhost:4000" \
--file-ignore="/^./_site/engine/api/.*$/,./_site/registry/configuration/index.html" \
--url-ignore="/^/docker-hub/api/latest/.*$/,/^/engine/api/v.+/#.*$/,/^/glossary/.*$/"
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 moved the htmlproofer check in our Dockerfile so we can also check locally.

@crazy-max crazy-max requested a review from thaJeztah May 6, 2022 19:30
@crazy-max crazy-max force-pushed the enhanced-dockerfile branch 3 times, most recently from 6696ddd to e19b527 Compare May 7, 2022 01:27
@crazy-max crazy-max force-pushed the enhanced-dockerfile branch 6 times, most recently from 5ac3791 to baed1ac Compare May 8, 2022 21:31
@crazy-max
Copy link
Member Author

Since this PR and switch to Jekyll 4.2.2, there is a conflict being reported: https://app.netlify.com/sites/docsdocker/deploys/62783880dcb06f000a4f7d25#L243

image

Looks like we push an index.html file which collides with a markdown file: https://github.com/docker/docker.github.io/tree/master/docker-hub/api. I guess we should remove https://github.com/docker/docker.github.io/tree/master/docker-hub/api/latest

@thaJeztah
Copy link
Member

Looks like we push an index.html file which collides with a markdown file: https://github.com/docker/docker.github.io/tree/master/docker-hub/api. I guess we should remove https://github.com/docker/docker.github.io/tree/master/docker-hub/api/latest

Yes, that looks a bit suspicious. https://github.com/docker/docker.github.io/blob/master/docker-hub/api/latest/index.html looks to have a redirect in it (but the redirect is to the page itself? Given that the latest.md includes a template/layout to use, I think the latest.md is the correct one.

Comment on lines -3 to -6
# Update me once in a while: https://github.com/github/pages-gem/releases
# Please ensure, before upgrading, that this version exists as a tag in starefossen/github-pages here:
# https://hub.docker.com/r/starefossen/github-pages/tags/
#
Copy link
Member

Choose a reason for hiding this comment

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

Any of this information useful to keep somewhere?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not relevant anymore since we use a vanilla Ruby image.

Comment on lines -16 to 15

# Engine
ARG ENGINE_BRANCH="20.10"
Copy link
Member

Choose a reason for hiding this comment

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

Should we use the new format to describe arguments (to allow the docker buildx outline feature to describe things?)

Copy link
Member Author

@crazy-max crazy-max May 9, 2022

Choose a reason for hiding this comment

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

Yes we could but these args will be removed in this follow-up if we are ok with it. See crazy-max/docker-docs@enhanced-dockerfile...crazy-max:fetch-remote-plugin

This comment was marked as outdated.

- ee
- index.html
- js/metadata.json
- LICENSE
Copy link
Member

Choose a reason for hiding this comment

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

Does this exclude LICENSE from the final image? (wondering if we need to keep it)

Copy link
Member Author

@crazy-max crazy-max May 9, 2022

Choose a reason for hiding this comment

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

Yes was not sure if it's relevant to have the LICENSE and README.md on the website actually:

Maybe better in a separate PR for this one?

@crazy-max crazy-max force-pushed the enhanced-dockerfile branch from baed1ac to a272949 Compare May 9, 2022 13:08
@crazy-max
Copy link
Member Author

You might be interested by this one @StefanScherer if you want to take a look

@crazy-max crazy-max force-pushed the enhanced-dockerfile branch from a272949 to 2311283 Compare May 13, 2022 12:00
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, thanks!

Comment on lines 66 to 87
PLATFORMS
x86_64-linux
Copy link
Member

Choose a reason for hiding this comment

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

Will this affect things when building on an M1 ? (or do we not have an arm64 image either way?)

Copy link
Member Author

@crazy-max crazy-max May 13, 2022

Choose a reason for hiding this comment

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

Afaik it will not break anything. It just says that this lock has been generated on this platform. We can add more of them which should be supported using bundle lock --add-platform .... As I can see the official ruby image support a bunch of platforms for the 2.6.10 tag we use:

image

So I would say we could add them. Or at least linux/arm64.

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 tested on M1 and it just adds aarch64-linux to the list of platforms in the lock file. I've updated the lock file with it.

Copy link
Member Author

@crazy-max crazy-max May 13, 2022

Choose a reason for hiding this comment

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

Also tried building on raspberry which seems to work fine. So I also added arm-linux.

$ uname -a
Linux foo 5.10.63-v7l+ #1459 SMP Wed Oct 6 16:41:57 BST 2021 armv7l GNU/Linux

Copy link
Member

Choose a reason for hiding this comment

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

Oh! Living on the edge; building it on a rPI! 😂

@crazy-max crazy-max force-pushed the enhanced-dockerfile branch 2 times, most recently from bbf4303 to 5287e71 Compare May 13, 2022 19:25
Signed-off-by: CrazyMax <crazy-max@users.noreply.github.com>
crazy-max added 6 commits May 13, 2022 22:07
Signed-off-by: CrazyMax <crazy-max@users.noreply.github.com>
Signed-off-by: CrazyMax <crazy-max@users.noreply.github.com>
Signed-off-by: CrazyMax <crazy-max@users.noreply.github.com>
Signed-off-by: CrazyMax <crazy-max@users.noreply.github.com>
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 enhanced-dockerfile branch from 5287e71 to 0f493a1 Compare May 13, 2022 20:08
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!

Copy link
Member

@StefanScherer StefanScherer left a comment

Choose a reason for hiding this comment

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

LGTM

@crazy-max crazy-max merged commit a717202 into docker:master May 18, 2022
@crazy-max crazy-max deleted the enhanced-dockerfile branch May 18, 2022 14:04
@thaJeztah
Copy link
Member

Thanks!

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.

4 participants