-
Notifications
You must be signed in to change notification settings - Fork 441
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add OpenAPI #10186
Add OpenAPI #10186
Conversation
2f26010
to
06937d9
Compare
I remember seeing a psalm failure with not enough memory at some point, I believe it's a memory leak that nukes psalm. Just giving it more ram didn't fix it, although I don't have this problem locally 🤔 |
06937d9
to
1227da7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Psalm breakage is not acceptable.
Also locally it does no longer finish and runs out of memory after allocating 8GB !
1227da7
to
7593377
Compare
Rebased on master to resolve conflicts and check if newer psalm version fixes the memory crash. Psalm still works perfectly on my machine 🤷♀️ |
Ok here are my investigation results:
I will update this comment when I have new findings. |
I found out, that even with old dependencies the problem existed. So I rebased this PR and turned around the stuff:
Turns out, after 1 psalm still worked, but not after 2 So I started a git bisect:
So I had a look at that commit and found the problem:
from After some testing the following patch also fixes it: diff --git a/lib/Controller/MatterbridgeController.php b/lib/Controller/MatterbridgeController.php
index 65e15eb1f..aac46265a 100644
--- a/lib/Controller/MatterbridgeController.php
+++ b/lib/Controller/MatterbridgeController.php
@@ -40,6 +40,7 @@ use OCP\IRequest;
* @psalm-import-type SpreedMatterbridge from ResponseDefinitions
* @psalm-import-type SpreedMatterbridgeParts from ResponseDefinitions
* @psalm-import-type SpreedMatterbridgeProcessState from ResponseDefinitions
+ * @psalm-import-type SpreedMatterbridgeWithProcessState from ResponseDefinitions
*/
class MatterbridgeController extends AEnvironmentAwareController {
@@ -56,7 +57,7 @@ class MatterbridgeController extends AEnvironmentAwareController {
/**
* Get bridge information of one room
*
- * @return DataResponse<Http::STATUS_OK, SpreedMatterbridge&SpreedMatterbridgeProcessState, array{}>
+ * @return DataResponse<Http::STATUS_OK, SpreedMatterbridgeWithProcessState, array{}>
*/
#[NoAdminRequired]
#[RequireLoggedInModeratorParticipant]
diff --git a/lib/ResponseDefinitions.php b/lib/ResponseDefinitions.php
index 49b82c41a..664aa7284 100644
--- a/lib/ResponseDefinitions.php
+++ b/lib/ResponseDefinitions.php
@@ -209,6 +209,8 @@ namespace OCA\Talk;
* running: bool,
* }
*
+ * @psalm-type SpreedMatterbridgeWithProcessState = SpreedMatterbridge&SpreedMatterbridgeProcessState
+ *
* @psalm-type SpreedSignalingSettings = array{
* helloAuthParams: array{
* "1.0": array{ |
So now that we are able to solve the problem, I will take over and migrate the responses to the expected namespace and see where we can simply the things. |
Thanks for the debugging. The openapi-extractor still doesn't support reading the namespace as per nextcloud/openapi-extractor#11 |
Since this PR there have been a few fixes in openapi-extractor, so please upgrade it to the latest version before continuing. |
Neither the version it seems (locking 0.0.1 while talk is at 18.0…). Will try to have a look |
7593377
to
5451424
Compare
The version field does not indicate the version of the software but the version of the document instead. This is a common misconception, but the right way to do it (until I've found a better solution) |
Okay, so it updates automatically everytime something changes? Or is hardcoded? Because it should reflect to breaking changes (api removed), etc I guess? |
Signed-off-by: jld3103 <jld3103yt@gmail.com>
Signed-off-by: jld3103 <jld3103yt@gmail.com>
Signed-off-by: jld3103 <jld3103yt@gmail.com>
Signed-off-by: jld3103 <jld3103yt@gmail.com>
Signed-off-by: jld3103 <jld3103yt@gmail.com>
Signed-off-by: jld3103 <jld3103yt@gmail.com>
Signed-off-by: jld3103 <jld3103yt@gmail.com>
Signed-off-by: jld3103 <jld3103yt@gmail.com>
Signed-off-by: jld3103 <jld3103yt@gmail.com>
a77c179
to
1b30c44
Compare
Signed-off-by: jld3103 <jld3103yt@gmail.com>
Signed-off-by: jld3103 <jld3103yt@gmail.com>
Signed-off-by: jld3103 <jld3103yt@gmail.com>
Signed-off-by: jld3103 <jld3103yt@gmail.com>
Signed-off-by: jld3103 <jld3103yt@gmail.com>
Signed-off-by: jld3103 <jld3103yt@gmail.com>
Signed-off-by: jld3103 <jld3103yt@gmail.com>
Signed-off-by: jld3103 <jld3103yt@gmail.com>
Signed-off-by: jld3103 <jld3103yt@gmail.com>
Signed-off-by: jld3103 <jld3103yt@gmail.com>
Signed-off-by: jld3103 <jld3103yt@gmail.com>
Signed-off-by: Joas Schilling <coding@schilljs.com>
Signed-off-by: Joas Schilling <coding@schilljs.com>
Signed-off-by: Joas Schilling <coding@schilljs.com>
Signed-off-by: Joas Schilling <coding@schilljs.com>
1b30c44
to
a8a21d5
Compare
After aligning with Kate in the chat, I squashed the fixups, kept my changes in individual commits for easier retrospective and rebased it. |
☑️ Resolves
Adds all the utilities and annotations required for OpenAPI.
🏁 Checklist
docs/
has been updated or is not required