fix: add lastmod entries to sitemaps - #2875
Conversation
Demo
Jenkins 
demos.haus 
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2875 +/- ##
==========================================
- Coverage 74.64% 74.48% -0.16%
==========================================
Files 19 20 +1
Lines 2374 2450 +76
==========================================
+ Hits 1772 1825 +53
- Misses 602 625 +23
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
immortalcodes
left a comment
immortalcodes
left a comment
There was a problem hiding this comment.
I have left some comments for the current approach.
I would also like to suggest an alternative approach where we save the manifest file for path-date date from git and instead of serving the sitemap files at the runtime, we generate them and pack them as static files inside the OCI-image.
| if not path.startswith(TEMPLATES_PATHSPEC + "/"): | ||
| # --name-only lists every file touched by the commit, not | ||
| # just ones under our pathspec; skip the rest. | ||
| continue |
There was a problem hiding this comment.
Is it not already covered when in line 48 above when you mention the TEMPLATE_PATHSPEC in git process
| def build_manifest(): | ||
| """ | ||
| Walk git history once and record, for every file ever committed | ||
| under templates/, the commit date of the most recent commit that | ||
| touched it. | ||
| """ | ||
| result = subprocess.run( | ||
| [ | ||
| "git", | ||
| "log", | ||
| f"--format={RECORD_MARKER}%cs", | ||
| "--name-only", | ||
| "--", | ||
| TEMPLATES_PATHSPEC, | ||
| ], | ||
| cwd=REPO_ROOT, | ||
| capture_output=True, | ||
| text=True, | ||
| check=True, | ||
| ) | ||
|
|
||
| manifest = {} | ||
| date = None | ||
| for line in result.stdout.splitlines(): | ||
| if line.startswith(RECORD_MARKER): | ||
| date = line[len(RECORD_MARKER) :] | ||
| continue | ||
|
|
||
| path = line.strip() | ||
| if not path.startswith(TEMPLATES_PATHSPEC + "/"): | ||
| # --name-only lists every file touched by the commit, not | ||
| # just ones under our pathspec; skip the rest. | ||
| continue | ||
|
|
||
| rel_path = path[len(TEMPLATES_PATHSPEC) + 1 :] | ||
| # git log is newest-first, so the first commit we see touching | ||
| # a path is its most recent change. | ||
| manifest.setdefault(rel_path, date) | ||
|
|
||
| return manifest |
There was a problem hiding this comment.
We are literally traversing the whole git history here for the repository which will become time-consuming as the commits grow.
What I would do is instead of building this whole file again at the build-time, I would keep the file in the repository, tag it with the date it was last built and then run the workflow to update it with commits that have taken place after that date-time
There was a problem hiding this comment.
I don't think webapp/ is the best location for this file, we might want to move it to say scripts/ for example, because this is essentially a build step and not part of flask app.
| Load templates/lastmod-manifest.json: a map of template file path | ||
| (relative to templates/) to the date it was last changed in git. | ||
|
|
||
| The production image doesn't ship .git (or the git history needed to | ||
| read it), so this manifest -- generated from git log by | ||
| scripts/generate-lastmod-manifest.py before the app is packaged -- is | ||
| how sitemap lastmod dates survive into the running app. Cached for | ||
| the life of the process: it's a static build artefact. |
There was a problem hiding this comment.
While I understand the detailed comments are useful, we can trim these comments to just couple of lines or even format our code in a way that comments are not needed at all, bigger comments increase the file size and also consume more time to read.
The code should itself act a understanding of itself.
| ("https://canonical.com/company", "company/index.html"), | ||
| ("https://canonical.com/knowledge", "knowledge/index.html"), | ||
| ] | ||
|
|
There was a problem hiding this comment.
It would not be a good idea to populate these dicts in the app.py, may be we abstract the dicts into separate yaml?
| CAREERS_STATIC_SITEMAP_PAGES = [ | ||
| ("https://canonical.com/careers", "careers/index.html", "monthly"), | ||
| ( | ||
| "https://canonical.com/careers/career-explorer", | ||
| "careers/career-explorer.html", | ||
| "monthly", | ||
| ), | ||
| ("https://canonical.com/careers/all", "careers/all.html", "weekly"), | ||
| ( | ||
| "https://canonical.com/careers/hiring-process", | ||
| "careers/hiring-process/index.html", | ||
| "weekly", | ||
| ), | ||
| ( | ||
| "https://canonical.com/careers/company-culture/remote-work", | ||
| "careers/company-culture/remote-work.html", | ||
| "monthly", | ||
| ), | ||
| ( | ||
| "https://canonical.com/careers/company-culture/progression", | ||
| "careers/company-culture/progression.html", | ||
| "monthly", | ||
| ), | ||
| ( | ||
| "https://canonical.com/careers/company-culture/diversity", | ||
| "careers/company-culture/diversity.html", | ||
| "monthly", | ||
| ), | ||
| ( | ||
| "https://canonical.com/careers/company-culture/sustainability", | ||
| "careers/company-culture/sustainability.html", | ||
| "monthly", | ||
| ), | ||
| ] |
There was a problem hiding this comment.
Also do we need to manually add these paths? As far as I remember, were we not using sitemap generator to dynamically generate paths?
cc: @britneywwc
| pages = [ | ||
| { | ||
| "url": url, | ||
| "last_modified": get_file_last_modified(TEMPLATES_DIR / path), | ||
| } | ||
| for url, path in PARTNERS_SITEMAP_PAGES | ||
| ] | ||
|
|
||
| xml_sitemap = flask.render_template("partners/sitemap.xml", pages=pages) | ||
| response = flask.make_response(xml_sitemap) |
There was a problem hiding this comment.
I suggest we should also abstract the sitemap generation functions in a separate file and then import and call them here, this makes the entire code more modular and clean.
| def get_knowledge_last_modified(sections): | ||
| """ | ||
| Get the last-modified date for the /knowledge index page: the most | ||
| recent of its own template and every section it links to. | ||
|
|
||
| Args: | ||
| sections: list of section dicts, as returned by | ||
| get_knowledge_sections() | ||
|
|
||
| Returns: | ||
| An ISO 8601 date string (YYYY-MM-DD) | ||
| """ | ||
| templates_dir = Path(flask.current_app.root_path).parent / "templates" | ||
| index_file = templates_dir / "knowledge" / "index.html" | ||
|
|
||
| last_modified = get_file_last_modified(index_file) | ||
| if sections: | ||
| last_modified = max( | ||
| last_modified, *(s["last_modified"] for s in sections) | ||
| ) | ||
|
|
||
| return last_modified |
There was a problem hiding this comment.
We can actually generalize it by passing in the path and then it can basically do the last modified for any parent path.
| - name: Pre-generate sitemap lastmod manifest | ||
| run: python3 webapp/lastmod_manifest.py generate-lastmod |
There was a problem hiding this comment.
Were you able to test it on staging? I think you would have to make changes to rockcraft.yaml so that it actually copies the generated manifest inside the OCI-image.
3 participants
Done
lastmodfield generation.QA
Issue / Card
https://warthogs.atlassian.net/browse/WD-38046