feat: more required changes for oc11#1538
Conversation
360d3c0 to
b57986e
Compare
b57986e to
e15e390
Compare
|
This PR is ready to merge BUT still so many things are uncovered/untouched. Open: Maintenance and Enterprise, envvar list page |
DeepDiver1975
left a comment
There was a problem hiding this comment.
🤖 Automated review by Claude Code review agent.
Overview
This PR (feat: more required changes for oc11) is a follow-up to #1511 and adapts the admin manual for ownCloud Server 11's Docker-only deployment model. The dominant themes are sound and consistent:
- Replace
config.phpediting with environment variables (e.g.OWNCLOUD_LOST_PASSWORD_LINK,OWNCLOUD_QUOTA_INCLUDE_EXTERNAL_STORAGE,OWNCLOUD_LOGIN_POLICY_ORDER,OWNCLOUD_ENABLE_CERTIFICATE_MANAGEMENT,OWNCLOUD_ENABLE_OIDC_REWRITE_URL). - Replace "install the app from the marketplace" with "enable the app" (apps are shipped in the image).
- Replace
/var/www/owncloud/datawith the container path/mnt/data/files(OWNCLOUD_VOLUME_FILES). - Rename "clients" to "Desktop and Mobile Apps".
- Add a new Memcached caching backend section.
- Cosmetic: collapse multi-line paragraphs into single lines, normalize image widths to 350, fix a dead link.
Overall the direction is correct and the changes are coherent. A few concrete issues below should be fixed before merge.
Potential issues / risks (most important first)
-
Broken xref anchor — caching page (
caching_configuration.adoc). The cache list adds* xref:memcached[Memcached]and the OIDC requirements referencexref:memcached-style targets, but the new section header is=== Memcached. AsciiDoc auto-generates the ID_memcached(leading underscore) for=== Memcachedunless an explicit anchor is set. The existingxref:redis[Redis]works because there is presumably an explicit[#redis]/[[redis]]anchor. Please add an explicit anchor[#memcached]above=== Memcached(and verify the=== Memcached Configuration/ "Clearing the Memcached Cache" targets too), otherwisexref:memcached[Memcached]will be a broken link. -
Double slash in path examples (
user_management.adoc). Two introduced paths have a doubled slash:Here you can see, that the home of user lisa is located in /mnt/data/files//lisa(notefiles//lisa).{occ-command-example-prefix-docker} user:home:list-users /mnt/data/files/— trailing slash here is inconsistent with theuser:list ... /mnt/data/files/lisaoutput above (no trailing slash) and withuser:home:list-dirsoutput- /mnt/data/files(no trailing slash). Normalize to/mnt/data/files/lisaand/mnt/data/files.
-
Grammar / broken sentence (
user_oauth2.adoc).OAuth2 functionality is available \OAuth2` app which is is shipped with the ownCloud image.— missing a word after "available" (e.g. "via the") and a doubled "is is". Should read: "OAuth2 functionality is available via theOAuth2` app which is shipped with the ownCloud image." -
Typo
redifined(user_auth_ldap.adoc). "...part of the Docker volume and can be redifined if required." →redefined. -
Source language label
.envis unusual. Several blocks use[source,.env](leading dot), e.g. inimport_ssl_cert.adoc,login_policies.adoc,oidc.adoc,reset_admin_passwordarea,caching_configuration.adoc. Verify the project's Antora/highlighter recognizes.env(other code blocks in the same files use[source,bash],[source,php],[source,apache]without a leading dot). If.envis an established convention in this repo it is fine, but it should be consistent — note thatreset_user_password.adockeeps[source,php]for what is now an environment variable (OWNCLOUD_LOST_PASSWORD_LINK=...), which is the wrong language label and inconsistent with the rest of the PR.
Code quality / style
-
caching_configuration.adoc: the new NOTE blockNOTE: The `memcached` PHP extension is already embedded in the Docker image and enabled.has trailing whitespace on the following (now blank-ish) line containing two spaces. Harmless but worth cleaning.
-
caching_configuration.adoctypo in prose: "Redis is used of the Docker Compose deployment example" — pre-existing line shown in context ("is used of") reads oddly ("in" intended); not introduced by this PR but adjacent. -
import_ssl_cert.adoc: good catch replacing the malformed_CERTIFICATE_MANAGEMENT=true(leading underscore, no prefix) withOWNCLOUD_ENABLE_CERTIFICATE_MANAGEMENT=true. -
config_sample_php_parameters.adoc: replacing the deadapprize.infolink with the officialhttps://www.php.net/manual/en/memcached.constants.phpis a solid improvement. -
oidc.adoc: the xref anchor was changed from#define-how-to-relax-same-site-cookie-settingsto#define-how-to-relax-the-same-site-cookie-settings(added "the"). Please confirm the target anchor inconfig_sample_php_parameters.adocmatches the new form, otherwise this xref breaks. The conversion of the list from-markers to*and the--open-block wrapping look correct. -
Removing the OIDC client-support version table (Desktop >= 2.7.0, Android >= 2.15, iOS >= 1.2) loses potentially useful minimum-version info. If that table is intentionally dropped because all current clients support OIDC, fine; just flagging the information loss.
-
reset_admin_password.adoc: removing the HTTP-user/occ-as-www-data guidance is appropriate for the Docker-only model (occ runs via the documented prefix). Good. -
docker-compose.yml: the commented-out Memcached service block has inconsistent indentation (memcached:indented two spaces but its keys only one), which differs from the activeredis/services indentation. Since it is commented out it won't break parsing, but if a user uncomments it the YAML will be malformed. Recommend aligning to the same indentation as the real services.
Specific suggestions (summary)
- Add explicit
[#memcached]anchor for thexref:memcached[...]links. - Fix
/mnt/data/files//lisaand the trailing-slash inconsistencies. - Fix "available
OAuth2app which is is shipped" sentence. - Fix
redifined→redefined. - Verify the
.envsource label and the renamed#define-how-to-relax-the-same-site-cookie-settingsanchor resolve; makereset_user_password.adocuse the same.envlabel as the rest of the PR. - Align indentation of the commented Memcached compose block.
Nothing here is blocking from a content-accuracy standpoint; the xref anchor (#1) and the path double-slash (#2) are the items most likely to produce a visibly broken doc and should be confirmed before merge.
- user_management: remove doubled slash in /mnt/data/files//lisa and drop inconsistent trailing slash in user:home:list-users example - user_oauth2: fix broken sentence "available `OAuth2` app which is is" - user_auth_ldap: fix typo redifined -> redefined - caching_configuration: use [source,php] instead of non-standard [source,.php], drop trailing whitespace after the memcached NOTE, and fix "Redis is used of" -> "used in" - reset_user_password: label the env var block [source,.env] to match the rest of the oc11 changes - docker-compose: align indentation of the commented memcached service The reported broken xref:memcached anchor and the renamed same-site cookie anchor were verified as correct (idprefix is empty globally, so section titles auto-generate matching IDs) and left unchanged. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Thomas Müller <1005065+DeepDiver1975@users.noreply.github.com>
DeepDiver1975
left a comment
There was a problem hiding this comment.
🤖 Automated review by Claude Code review agent (round 2, against b7a7bbb).
Overview
This PR adapts the admin manual for ownCloud Server 11's Docker-only model. The themes are consistent and correct:
- Replace
config.phpsnippets with environment variables (OWNCLOUD_LOST_PASSWORD_LINK,OWNCLOUD_QUOTA_INCLUDE_EXTERNAL_STORAGE,OWNCLOUD_LOGIN_POLICY_ORDER,OWNCLOUD_ENABLE_CERTIFICATE_MANAGEMENT,OWNCLOUD_ENABLE_OIDC_REWRITE_URL,OWNCLOUD_HTTP_COOKIE_SAMESITE, …). - Replace "install the app from the marketplace" with "enable the app" (apps ship in the image).
- Replace
/var/www/owncloud/datawith the container path/mnt/data/files(OWNCLOUD_VOLUME_FILES). - Rename "clients" → "Desktop and Mobile Apps".
- Add a Memcached caching backend section.
- Cosmetic: collapse multi-line paragraphs, normalize image widths to 350, fix a dead link.
The fixes from the previous review round have been applied correctly (double-slash, is is, redifined, [source,.php], NOTE whitespace, compose indentation, .env label on the password-reset block). The earlier "broken xref:memcached anchor" concern is not an issue — idprefix is empty globally, so === Memcached auto-generates the id memcached and the xref resolves. Good direction overall; a few new items below.
Potential issues / risks
-
Heading level inconsistency —
caching_configuration.adoc.=== Redis Configurationis a level-3 heading (under== Configuring Memory Caching), but the new== Memcached Configurationis level-2, breaking out of that parent section. As a result=== Clearing the Memcached Cache(level 3) nests under the level-2 Memcached block while its Redis counterpart==== Clearing the Redis Cachesits at level 4 under=== Redis. The two backends should be structured symmetrically. Suggest=== Memcached Configuration(level 3) to match=== Redis Configuration, and adjustClearing the Memcached Cacheaccordingly. -
Two typos in
installing_with_docker.adoc(new prose):- "are used to the define the admin user" → "are used to define the admin user".
- "After the inital logon" → "initial".
-
Malformed multi-host Memcached example —
caching_configuration.adoc. The commented-out (////) NOTE has unbalanced quotes:OWNCLOUD_MEMCACHED_HOST="'host_1',port1,host_2',port_2"—'host_1'is quoted buthost_2'has only a trailing quote, andport1vsport_2are inconsistent. Compare to the workingOWNCLOUD_REDIS_SEEDS='host_1:port,host_2:port'later in the same file. Since it's inside////it won't render today, but it should be corrected before it's ever un-commented.
Minor / style
user_management.adoc: "The directorymnt/datais used as Docker volume…" is missing a leading slash (/mnt/data) and reads better as "is used as a Docker volume".oidc.adoc: dropping the OIDC client-support version table (Desktop ≥ 2.7.0, Android ≥ 2.15, iOS ≥ 1.2) loses minimum-version info. Fine if intentional (all current clients support OIDC) — just flagging the information loss.config_sample_php_parameters.adoc: replacing the deadapprize.infolink withhttps://www.php.net/manual/en/memcached.constants.phpis a solid improvement.import_ssl_cert.adoc: good catch fixing the malformed_CERTIFICATE_MANAGEMENT=true→OWNCLOUD_ENABLE_CERTIFICATE_MANAGEMENT=true.oidc.adoc: the renamed xref#define-how-to-relax-the-same-site-cookie-settingsresolves correctly against the target heading. 👍
Verification notes
.envsource label: established convention across the repo — no concern.- All xref anchors touched in this PR (
#memcached,#define-how-to-relax-the-same-site-cookie-settings) were checked and resolve.
None of the above is content-blocking. Item #1 (heading levels) is the one most worth fixing since it affects the rendered page structure/TOC; #2 and #3 are quick text fixes.
- caching_configuration: make Memcached section heading levels symmetric with Redis (=== Memcached Configuration, ==== Clearing the Memcached Cache) so the page structure/TOC nests correctly; fix unbalanced quotes and inconsistent port naming in the commented multi-host example - installing_with_docker: fix typos "used to the define" -> "used to define" and "inital" -> "initial" - user_management: add missing leading slash and article in "/mnt/data is used as a Docker volume" Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Thomas Müller <1005065+DeepDiver1975@users.noreply.github.com>
DeepDiver1975
left a comment
There was a problem hiding this comment.
🤖 Automated review by Claude Code review agent (round 3, against d07f062).
Overview
Round-2 items are resolved: Memcached heading levels are now symmetric with Redis (=== / ====), the installing_with_docker typos are fixed, the commented multi-host Memcached example is balanced, and the /mnt/data prose reads correctly. This round focuses on remaining smaller inconsistencies the env-var migration introduced.
Potential issues
- Quota prose now inconsistent with its converted example —
user_management.adoc. The code block was correctly migrated toOWNCLOUD_QUOTA_INCLUDE_EXTERNAL_STORAGE=false, but the sentence directly above still describes the oldconfig.phpmodel:"You now have a configurable option in
config.phpthat controls whether external storage is counted… If you prefer to include it, then change the defaultfalsetotrue.:"
Two problems: (a) it still says "inconfig.php" though the example is now an env var; and (b) the trailing ".:" (period immediately followed by colon) is malformed punctuation. Suggest rewording to reference the environment variable and set the value totrueto include external storage, and fix the.:.
Minor / style
- Trailing whitespace on three lines rewritten by this PR (
user_auth_ldap.adoc: the "…mapping from an ownCloud username to an LDAP user. " and "…leftovers everywhere. " lines; and the "and look for " line). Harmless but worth trimming since these lines were already being edited. user_auth_ldap.adoc: "The connection port of the backup LDAP server. If no port…" has a double space after the period (introduced when collapsing the multi-line block).
Verified clean
- All xref anchors touched in the PR resolve (
#memcached,#define-how-to-relax-the-same-site-cookie-settings). .envsource label is an established repo convention.- Memcached section structure/TOC now nests correctly.
- Port
11211is used consistently across the Memcached config and the telnet flush example.
Nothing blocking. Item #1 is the only content-level inconsistency; the rest is whitespace polish.
- user_management: reword the external-storage quota prose to reference the environment variable instead of config.php (the example was already converted to OWNCLOUD_QUOTA_INCLUDE_EXTERNAL_STORAGE) and fix the malformed ".:" punctuation - user_auth_ldap: trim trailing whitespace and a double space on lines rewritten by this PR Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Thomas Müller <1005065+DeepDiver1975@users.noreply.github.com>
DeepDiver1975
left a comment
There was a problem hiding this comment.
🤖 Automated review by Claude Code review agent (round 4, against 3dc0702).
Result: clean — no actionable findings
Round-3 items are resolved (quota prose now references the environment variable and the .: punctuation is fixed; trailing/double whitespace in user_auth_ldap.adoc trimmed). I re-scanned the full diff and found no remaining issues:
- AsciiDoc structure: all open-block (
--) delimiters are balanced in every restructured file (oidc.adoc,caching_configuration.adoc,user_auth_ldap.adoc,installing_with_docker.adoc); list continuations (+) are well-formed. - Headings: Memcached section nests symmetrically with Redis.
- xrefs / images / anchors: all touched targets resolve; no malformed
xref:/image:macros in added lines. - Prose: no doubled words, no trailing whitespace, no
.:-style punctuation left. config.phpreferences remaining in changed files are correct, not stale: the OIDC app legitimately still usesconfig.php(clustered setups fall back to it), andconfig_sample_php_parameters.adocdocumentsconfig.phpitself. The logging-pagedatadirectorymention is immediately clarified by theOWNCLOUD_VOLUME_FILESsentence below it.
The review/fix cycle has converged — LGTM from a documentation-correctness and consistency standpoint. No fix commit needed this round.
DeepDiver1975
left a comment
There was a problem hiding this comment.
🤖 Automated review by Claude Code review agent.
Overview
Follow-up PR to #1511, converting more admin-manual pages to the Docker-only / environment-variable deployment model for ownCloud Server 11. Changes span 20 files: config snippets switched from config.php to env vars, Memcached caching documentation added, OIDC/OAuth wording aligned to "apps" instead of "clients", a large LDAP page reflow, and many image width= normalizations to 350.
Since the last review (96af957 → 3dc0702): the new commit appears to be a clean-up pass that resolves the issues flagged previously. See the status table below.
Status of previously-flagged issues
| Issue | Status |
|---|---|
Broken xref:memcached[Memcached] anchor |
Fixed. A matching === Memcached section now exists; with idprefix: '' / idseparator: '-' it resolves to the memcached ID. |
/mnt/data/files//lisa double-slash path |
Fixed. Now /mnt/data/files/lisa (single slash) in all occurrences. |
| Broken/incomplete OAuth2 sentence | Fixed. The OAuth2 app paragraph now reads as a complete sentence. |
| "redifined" typo | Fixed. Text now reads "can be redefined". |
Inconsistent .env source labels |
Resolved / consistent. [source,.php] typo corrected to [source,php]; [source,.env] is used uniformly and matches the repo's existing convention. |
Code quality / style
- Anchor change in
oidc.adoc(#define-how-to-relax-same-site-cookie-settings→#define-how-to-relax-the-same-site-cookie-settings) is correct — it now matches the target heading "Define how to relax the same site cookie settings" under the repo's hyphenated auto-ID scheme. - New Memcached content (caching backend list entry,
=== Memcachedsection,=== Memcached Configuration, "Clearing the Memcached Cache") is internally consistent and the env-var triplet (OWNCLOUD_MEMCACHED_ENABLED/HOST/PORT) mirrors the Redis block. - LDAP page reflow (collapsing multi-line list items to single lines, fixing
--open-block boundaries) reads cleanly; AsciiDoc block delimiters look balanced. - Image
widthnormalization to350is consistent across pages.
Specific suggestions (minor / non-blocking)
config_sample_php_parameters.adoc— the brokenapprize.infolink was replaced withhttps://www.php.net/manual/en/memcached.constants.php. Good. Confirm that page documents the connection options being referenced (it lists Memcached predefined constants/options), as the surrounding heading is "Define connection options for memcached".- OAuth2 closing line (
user_oauth2.adoc) — the final sentence ending inxref:...[Open Authentication (OAuth2)]has no trailing period. Trivial, but worth adding for consistency. - Memcached locking guidance —
caching_configuration.adocstrongly (and correctly) warns against Memcached for distributed locking, yet the auto-set block still shows'memcache.locking' => '\OC\Memcache\Memcached'when Memcached is enabled. Consider a one-line caution next to that block reinforcing that locking falls back to / should use the DB or Redis, to avoid a reader wiring Memcached locking despite the warning above. docker-compose.yml— the commented-outmemcachedservice block is fine as an example; just ensure the referenced "caching configuration documentation" comment points readers somewhere discoverable (it is prose-only, no link, which is acceptable in a YAML comment).
Potential issues / risks
- External link freshness: the Redis-vs-Memcached comparison link (
scalegrid.io/blog/redis-vs-memcached-2021-comparison/) is dated "2021"; acceptable, but flagged as a future bit-rot candidate. - Version-specific wording removed: several "Since ownCloud 8.0.10 / Starting with ownCloud 10.0 / Up to Guests 0.12.1" historical notes were dropped (e.g. LDAP home-folder enforcement, Guests blocklist). For an oc11-targeted manual this is reasonable, but double-check none of the removed version caveats are still relevant to supported upgrade paths.
- No broken xrefs detected in the changed hunks; nav wiring is unaffected (no nav files in this PR).
Overall: the new commit cleanly addresses all five previously-flagged issues and the added Memcached/OIDC content is accurate and consistent. The remaining items above are minor polish.
Referencing: #1511 (feat: oc11 to support docker only deployments)
This is a follow up PR to add more changes required for oc11