Skip to content
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

Update SDK to latest Slack version #145

Merged
merged 1 commit into from
Mar 30, 2022
Merged

Update SDK to latest Slack version #145

merged 1 commit into from
Mar 30, 2022

Conversation

matthewnessworthy
Copy link
Contributor

rm -Rf generated/*
vendor/bin/jane-openapi generate --config-file=.jane-openapi.php
./bin/slack-api-client-generator spec:generate-patch
./bin/slack-api-client-generator spec:update

@matthewnessworthy
Copy link
Contributor Author

@damienalexandre / @mpyw seems there is an incompatibility with jane-php, please advise

Run rm -rf generated && vendor/bin/jane-openapi generate --config-file=.jane-openapi.php

PHP Fatal error:  Uncaught ArgumentCountError: Too few arguments to function Jane\Component\JsonSchema\Generator\RuntimeGenerator::__construct(), 2 passed in /home/runner/work/slack-php-api/slack-php-api/vendor/jane-php/open-api-2/JaneOpenApi.php on line 44 and exactly 3 expected in /home/runner/work/slack-php-api/slack-php-api/vendor/jane-php/json-schema/Generator/RuntimeGenerator.php:27

@matthewnessworthy
Copy link
Contributor Author

@damienalexandre / @mpyw issue resolved after updating jane-php/open-api-2 to ^7.2

@damienalexandre
Copy link
Member

Thanks a lot @matthewnessworthy for your contributions 🎉

Tests are green on that last commit 👏 💚 ; I'm just wondering about the Jane error, @Korbeil do you have an idea what was going on? (see https://github.com/jolicode/slack-php-api/runs/5709079316?check_suite_focus=true).
Thanks 👍

@matthewnessworthy
Copy link
Contributor Author

@damienalexandre it was just "jane-php/open-api-2": "~7.1.5", using 7.1.x which did not have the validation changes introduced in jane-php 7.2, causing a function signature mismatch

@damienalexandre
Copy link
Member

I don't see the new specification in your PR.

https://github.com/jolicode/slack-php-api/blob/main/src/Command/UpdateSpecificationCommand.php#L46

Did you forgot to commit the resources/slack-openapi.json file?

@damienalexandre
Copy link
Member

In fact the official spec didn't move since october 2020: https://github.com/slackapi/slack-api-specs/blob/master/web-api/slack_web_openapi_v2.json

So you are not updating the spec, but adding a new manual overwrite, right?

@matthewnessworthy
Copy link
Contributor Author

I ran the following

rm -Rf generated/*
vendor/bin/jane-openapi generate --config-file=.jane-openapi.php
./bin/slack-api-client-generator spec:generate-patch
./bin/slack-api-client-generator spec:update

It did not modify resources/slack-openapi.json

This PR fixes the failing workflow step (jane-php/open-api-2 version mismatch) and pulls in any Slack changes
It's an attempt to unblock the way for my other 2 pull requests #146 and #147

@damienalexandre
Copy link
Member

Running the same commands as you I don't have any specification update (and that's the expected behavior as Slack didn't update their OpenAPI).

I think you mixed some stuffs.

Here is what should be done to fix the SDK generation:

+++ w/composer.json
@@ -34,7 +34,7 @@
         "php-http/multipart-stream-builder": "^1.1"
     },
     "require-dev": {
-        "jane-php/open-api-2": "~7.1.5",
+        "jane-php/open-api-2": "~7.1",
         "symfony/http-client": "^5.4 || ^6.0",
         "nyholm/psr7": "^1.2",
         "friendsofphp/php-cs-fixer": "^3.2.2",
composer update
rm -Rf generated/*
vendor/bin/jane-openapi generate --config-file=.jane-openapi.php

That's all.

Could you rebuild this Pull Request with this change only so we can merge and then look at the other changes?

Thanks a lot!

@matthewnessworthy
Copy link
Contributor Author

@damienalexandre no problem, done

@damienalexandre
Copy link
Member

Perfect, thanks a lot for this fix 👍

@damienalexandre damienalexandre merged commit 4479e68 into jolicode:main Mar 30, 2022
damienalexandre added a commit that referenced this pull request Mar 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants