Skip to content

Conversation

@crazy-max
Copy link
Member

follow-up #15250 (comment)

fix sitemap last modification date not being taken into account:

image

it will use git and fall back to file's mtime if necessary.

also adds following entries to sitemap exclusion list that should not be there: https://docs-stage.docker.com/sitemap.xml

image

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

@crazy-max crazy-max requested a review from thaJeztah July 29, 2022 15:31
@netlify
Copy link

netlify bot commented Jul 29, 2022

Deploy Preview for docsdocker ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit 7bba3a3
🔍 Latest deploy log https://app.netlify.com/sites/docsdocker/deploys/631608b2f2b14c0008366e6b
😎 Deploy Preview https://deploy-preview-15267--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 crazy-max requested a review from usha-mandya August 3, 2022 10:32
@crazy-max
Copy link
Member Author

crazy-max commented Aug 3, 2022

@thaJeztah @usha-mandya This PR would make search engines happy 🙂

@craig-osterhout craig-osterhout added the area/ux Issue affects functionality of docs.docker.com label Aug 18, 2022
@crazy-max crazy-max force-pushed the fix-sitemap-lastmod branch from 7c48d16 to a308fcd Compare August 31, 2022 11:20
@crazy-max
Copy link
Member Author

crazy-max commented Aug 31, 2022

@thaJeztah Except your concerns with .git folder being added, it LGTY?

I made some comparison in our CI to see if there is a huge perf regression:

Last build on master branch: https://github.com/docker/docker.github.io/runs/8111429336?check_suite_focus=true#step:4:571

#15 45.91                     done in 45.237 seconds.
#15 45.91  Auto-regeneration: disabled. Use --watch to enable.
#15 DONE 46.3s

This PR: https://github.com/docker/docker.github.io/runs/8116505924?check_suite_focus=true#step:4:577

#15 57.14                     done in 56.239 seconds.
#15 57.14  Auto-regeneration: disabled. Use --watch to enable.
#15 DONE 57.7s

It seems acceptable to me and worth the change to fix our sitemap so search engines can consume it and make relevant decision on results.

We could also generate sitemap only on CI with a new env so we are not impacting developers but I think it's fine as it is atm.

@crazy-max crazy-max force-pushed the fix-sitemap-lastmod branch 2 times, most recently from 16cf0a3 to 7bba3a3 Compare September 5, 2022 14:33
@usha-mandya
Copy link
Member

Thanks Kevin. @thaJeztah Does it LGTY?

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 fix-sitemap-lastmod branch from 7bba3a3 to 38709b0 Compare October 26, 2022 04:14
@netlify
Copy link

netlify bot commented Oct 26, 2022

Deploy Preview for docsdocker ready!

Name Link
🔨 Latest commit a16eff5
🔍 Latest deploy log https://app.netlify.com/sites/docsdocker/deploys/63592cc155308c00084648aa
😎 Deploy Preview https://deploy-preview-15267--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 fix-sitemap-lastmod branch from 38709b0 to 3d0858c Compare October 26, 2022 04:28
@crazy-max
Copy link
Member Author

crazy-max commented Oct 26, 2022

Made some changes related to #15239 (comment) so now remote resources are cached and fetched using Git so incremental builds are improved. This will speedup local builds by almost 62%:

Before:

#15 45.91                     done in 45.237 seconds.
#15 45.91  Auto-regeneration: disabled. Use --watch to enable.
#15 DONE 46.3s

Now:

#17 17.84                     done in 17.198 seconds.
#17 17.84  Auto-regeneration: disabled. Use --watch to enable.
#17 DONE 18.2s

With this new way to fetch remote resources we are now able to set the right last modification date for remote pages too. cc @usha-mandya @dvdksn

@crazy-max crazy-max force-pushed the fix-sitemap-lastmod branch 2 times, most recently from bdabffb to 6618615 Compare October 26, 2022 05:41
@dvdksn
Copy link
Contributor

dvdksn commented Oct 26, 2022

The incremental builds are really fast with this 🤩

But I noticed that the first build can take a really long time. Build time on main for a clean build is ~180 seconds for me, whereas in this branch it finished after ~240 seconds. I wonder if it's because we're cloning history? I tried setting git.clone(depth: 1) and it seems to fix the speed issue; I'm now back at ~180 seconds for a clean build. WDYT?

diff --git a/_plugins/fetch_remote.rb b/_plugins/fetch_remote.rb
index 4370baaccb..158fb3919a 100644
--- a/_plugins/fetch_remote.rb
+++ b/_plugins/fetch_remote.rb
@@ -55,11 +55,11 @@ module Jekyll
           rescue => e
             FileUtils.rm_rf(clonedir)
             puts "    Cloning repository into #{clonedir}"
-            git = Git.clone("#{entry['repo']}.git", Pathname.new(clonedir), branch: entry['ref'])
+            git = Git.clone("#{entry['repo']}.git", Pathname.new(clonedir), branch: entry['ref'], depth: 1)
           end
         else
           puts "    Cloning repository into #{clonedir}"
-          git = Git.clone("#{entry['repo']}.git", Pathname.new(clonedir), branch: entry['ref'])
+          git = Git.clone("#{entry['repo']}.git", Pathname.new(clonedir), branch: entry['ref'], depth: 1)
         end
 
         entry['paths'].each do |path|

@crazy-max
Copy link
Member Author

I tried setting git.clone(depth: 1) and it seems to fix the speed issue; I'm now back at ~180 seconds for a clean build. WDYT?

Ah good point yes we don't need the history anyway, will make this change 👍

@crazy-max crazy-max force-pushed the fix-sitemap-lastmod branch from 6618615 to ab80cc1 Compare October 26, 2022 12:25
@crazy-max crazy-max requested a review from dvdksn October 26, 2022 12:28
@crazy-max crazy-max force-pushed the fix-sitemap-lastmod branch from ab80cc1 to 9f82f5e Compare October 26, 2022 12:34
Signed-off-by: CrazyMax <crazy-max@users.noreply.github.com>
@crazy-max crazy-max force-pushed the fix-sitemap-lastmod branch from 9f82f5e to a16eff5 Compare October 26, 2022 12:49
Copy link
Contributor

@dvdksn dvdksn left a comment

Choose a reason for hiding this comment

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

:shipit:

@crazy-max crazy-max merged commit 0001d96 into docker:main Oct 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/ux Issue affects functionality of docs.docker.com

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants