fix(ci): drop nc31; add nc34#449
Open
oleksandr-nc wants to merge 5 commits into
Open
Conversation
Nextcloud 31 is removed from the analysis-coverage matrix and Nextcloud 34 takes its place across the OCI, PgSQL and SQLite/client jobs, keeping the job count unchanged. The Notes checkout condition is simplified to pin only stable32 to the 4.x line (Notes v5 requires NC >= 33). Signed-off-by: Oleksandr Piskun <oleksandr2088@icloud.com>
Creating a local circle through an ExApp (app mode) started returning HTTP 400 after the nextcloud/circles local-controller permission changes landed on the stable branches in mid-2026. The request is well-formed (client mode is unaffected), so this is an upstream Circles regression, not an nc_py_api bug. Mark the test xfail (non-strict) so CI stays green and an xpass flags the day Circles is fixed on the stable branches. Signed-off-by: Oleksandr Piskun <oleksandr2088@icloud.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughUpdates CI branch coverage and checkout handling, persists TalkBot secrets into ChangesCI Workflow and Test Updates
TalkBot Secret Persistence
Teams Test Xfail
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/analysis-coverage.yml (1)
548-554: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDead conditional branch: sqlite matrix never includes
stable32.The sqlite job matrix (Line 518) only contains
stable33/stable34, somatrix.nextcloud == 'stable32'here always evaluates false — the ref always resolves tomain. Functionally correct but the copy-pasted conditional is misleading; could simplify to a hardcodedmainfor this job.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/analysis-coverage.yml around lines 548 - 554, The conditional in the Checkout Notes step is dead for this sqlite job because the matrix only ever uses stable33 and stable34, so matrix.nextcloud == 'stable32' can never be true. Update the Checkout Notes configuration to remove the unreachable branch and reference the existing Checkout Notes step in the workflow, simplifying ref to the fixed target actually used here (main) for this job.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/analysis-coverage.yml:
- Line 518: The matrix entry for nextcloud uses bracket spacing that violates
YAMLlint. Update the nextcloud matrix definition to match the same compact
bracket style used elsewhere in the workflow, and check the surrounding matrix
keys in analysis-coverage.yml for consistent YAMLlint-compliant formatting.
- Line 341: Fix the YAML bracket spacing in the `nextcloud` matrix entry so it
matches YAMLlint expectations. Update the array formatting in the workflow
definition to remove the extra spaces inside the brackets while keeping the same
values (`stable32` and `stable34`) in the same `nextcloud` section.
In `@tests/actual_tests/teams_test.py`:
- Around line 325-330: The xfail on the affected test is too broad because it
will hide unrelated failures; tighten the marker in the teams test by adding a
raises= constraint to the pytest.mark.xfail around the ExApp local circle
creation case. Use the specific exception type thrown for the HTTP 400 path from
nc_py_api (for example the NextcloudException returned by the call in the test)
so only the documented Circles regression is treated as expected failure while
other test regressions still fail normally.
---
Outside diff comments:
In @.github/workflows/analysis-coverage.yml:
- Around line 548-554: The conditional in the Checkout Notes step is dead for
this sqlite job because the matrix only ever uses stable33 and stable34, so
matrix.nextcloud == 'stable32' can never be true. Update the Checkout Notes
configuration to remove the unreachable branch and reference the existing
Checkout Notes step in the workflow, simplifying ref to the fixed target
actually used here (main) for this job.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 611bfcf8-021e-4db3-abd4-7fbe7764b53c
📒 Files selected for processing (2)
.github/workflows/analysis-coverage.ymltests/actual_tests/teams_test.py
The new stable34 matrix entries failed during Nextcloud setup: - notifications since stable34 pulls a Composer dependency (web-push) and its composer/autoload.php now requires vendor/autoload.php, which a plain git checkout does not contain. Build the app when the checked-out branch needs it (older branches load without a vendor dir, so the step is a no-op there). - NC >= 34 sets the DB session timezone to Etc/UTC, which the EOL oracle-xe:11 image cannot resolve (ORA-01882). Keep the OCI job on stable33 until the Oracle image is bumped to 18+; stable34 stays covered by the PgSQL and SQLite jobs. Also drop the now dead stable32 branch from the sqlite Notes checkout ref. Signed-off-by: Oleksandr Piskun <oleksandr2088@icloud.com>
Add raises=NextcloudException so only the documented Circles HTTP 400 is treated as an expected failure; any other error in the test still fails normally. Signed-off-by: Oleksandr Piskun <oleksandr2088@icloud.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@nc_py_api/nextcloud.py`:
- Around line 390-395: The docstring for register_talk_bot should explicitly
tell direct consumers how to persist the returned secret instead of only
pointing to TalkBot.enabled_handler. Update the note to mention that callers who
do not use TalkBot/AsyncTalkBot must store the secret themselves with
appconfig_ex.set_value(bot_id, bot_secret, sensitive=True) or equivalent,
alongside the existing AppAPI 34+ behavior note, so the public method
documentation clearly covers the new compatibility requirement.
In `@nc_py_api/talk_bot.py`:
- Around line 133-140: The new appconfig persistence in `enabled_handler` can
throw after `register_talk_bot` has already succeeded, which can break the
enable flow and leave the bot half-configured. Update the `enabled_handler` path
around `nc.appconfig_ex.set_value` to catch and handle persistence failures
explicitly, so the secret is still set in `os.environ` and the failure is
surfaced as a clear warning or log message instead of propagating. Use the
existing `register_talk_bot`, `appconfig_ex.set_value`, and `enabled_handler`
symbols to keep the fix localized and make retries or later recovery possible.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 05d47b35-98fd-4071-b235-87dfef5342cf
📒 Files selected for processing (2)
nc_py_api/nextcloud.pync_py_api/talk_bot.py
oleksandr-nc
force-pushed
the
fix/ci-drop-nc31-add-nc34
branch
from
f4bbf65 to
643b1e6
Compare
AppAPI 34 moved TalkBot bookkeeping to a dedicated table and no longer mirrors the bot secret into `appconfig_ex`, which `get_bot_secret` reads back. So any ExApp process that did not itself register the bot (a second uvicorn worker, or the test's callback server) raised "Can't find the 'secret' of the bot" and never replied on NC 34. Persist the secret from `enabled_handler` under a nc_py_api-owned `appconfig_ex` key (a hash distinct from `bot_id`) and read it back as an extra fallback in `get_bot_secret`. The key is deliberately NOT `bot_id`: AppAPI <= 33 still owns and re-reads that key on re-registration, so writing there ourselves makes it mint a different secret and Talk rejects the re-install. This leaves 32/33 untouched and fixes multi-worker bots on 34. Signed-off-by: Oleksandr Piskun <oleksandr2088@icloud.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #449 +/- ##
==========================================
+ Coverage 94.67% 94.69% +0.01%
==========================================
Files 48 48
Lines 5636 5650 +14
==========================================
+ Hits 5336 5350 +14
Misses 300 300
🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Mark failed
circlestests withxfailSummary by CodeRabbit