-
Notifications
You must be signed in to change notification settings - Fork 58
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 workflows.* endpoints to use POST method instead of GET #147
Update workflows.* endpoints to use POST method instead of GET #147
Conversation
@damienalexandre please review |
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.
The change to the workflow API looks ok but the changes to the token are wrong IMO.
resources/slack-openapi-patched.json
Outdated
@@ -5138,6 +5140,7 @@ | |||
"description": "Authentication token. Requires scope: `admin.teams:write`", | |||
"in": "formData", | |||
"name": "token", | |||
"required": true, |
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.
Why is this added? I thinks it should not.
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.
they are automatic based on the commands i ran above
i assume it's correct as it's auto generating these particular changes
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.
this could be related to the validation changes introduced into jane-php 7.2, but this is not something i'm manually adding
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.
I confirm the token should never be required
because it would break how the token is automatically passed as a request header (see https://github.com/jolicode/slack-php-api/blob/main/src/ClientFactory.php#L40-L42).
Even if Slack allow to pass the token either in a header or in form data for those endpoints (according to their doc : "Tokens should be passed as an HTTP Authorization header or alternatively, as a POST parameter"), it should never be required on our patched spec to allow the token to be sent automatically by the Http client behind the hood.
So IMO if the versioned patch currently forces these required
(which is strange) we probably need to fix the patch
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.
i'll rebuild the PR and list the command i use, maybe i did something incorrectly the first time around
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.
PR rebuilt with only the specific changes to the workflows.* steps applied
then i ran ./bin/slack-api-client-generator spec:generate-patch
and vendor/bin/jane-openapi generate --config-file=.jane-openapi.php
and it compiles the PR as you see it now
running ./bin/slack-api-client-generator spec:update
on this branch or main
results in an error
./bin/slack-api-client-generator spec:update
Downloaded and saved a new specification version
Sorted the official specification by keys and merged User and Conversation objects
[ERROR] Could not apply the patch
In UpdateSpecificationCommand.php line 62:
The command "patch --verbose -p0 < resources/slack-openapi-sorted.patch -o resources/slack-openapi-patched.json" failed.
Exit Code: 1(General error)
Working directory: /Users/matthew/Sites/oam/slack-php-api
Output:
================
Hmm... Looks like a unified diff to me...
The text leading up to this was:
--------------------------
|--- resources/slack-openapi-sorted.json 2022-03-30 14:59:41.000000000 +0200
|+++ resources/slack-openapi-patched.json 2022-03-30 14:59:52.000000000 +0200
--------------------------
Patching file resources/slack-openapi-sorted.json using Plan A...
Hunk #1 succeeded at 397.
Hunk #2 succeeded at 510.
Hunk #3 succeeded at 564.
Hunk #4 succeeded at 829.
Hunk #5 succeeded at 931.
Hunk #6 succeeded at 1009.
Hunk #7 succeeded at 1071.
Hunk #8 succeeded at 1137.
Hunk #9 succeeded at 1558.
Hunk #10 succeeded at 1753.
Hunk #11 succeeded at 1873.
Hunk #12 succeeded at 2400.
Hunk #13 succeeded at 2504.
Hunk #14 succeeded at 2648.
Hunk #15 succeeded at 2803.
Hunk #16 succeeded at 2907.
Hunk #17 succeeded at 3042.
Hunk #18 succeeded at 3146.
Hunk #19 succeeded at 3276.
Hunk #20 succeeded at 3384.
Hunk #21 succeeded at 3494.
Hunk #22 succeeded at 3619.
Hunk #23 succeeded at 3717.
Hunk #24 succeeded at 3872.
Hunk #25 succeeded at 3999.
Hunk #26 succeeded at 4115.
Hunk #27 FAILED at 4230.
Hunk #28 succeeded at 4324.
Hunk #29 FAILED at 4430.
Hunk #30 succeeded at 4555.
Hunk #31 succeeded at 4713.
Hunk #32 succeeded at 4835.
Hunk #33 succeeded at 4925.
Hunk #34 succeeded at 5028.
Hunk #35 succeeded at 5348 with fuzz 1 (offset 217 lines).
Hunk #36 succeeded at 6368 (offset 1143 lines).
Hunk #37 FAILED at 6489.
Hunk #38 FAILED at 6585.
Hunk #39 FAILED at 6681.
Hunk #40 FAILED at 6782.
Hunk #41 FAILED at 6883.
Hunk #42 FAILED at 6979.
Hunk #43 FAILED at 7080.
Hunk #44 FAILED at 7180.
Hunk #45 FAILED at 7292.
Hunk #46 FAILED at 7388.
Hunk #47 FAILED at 7500.
Hunk #48 succeeded at 6449.
Hunk #49 succeeded at 6848 with fuzz 1 (offset 293 lines).
Hunk #50 FAILED at 6945.
Hunk #51 FAILED at 7042.
Hunk #52 FAILED at 7138.
Hunk #53 FAILED at 7235.
Hunk #54 succeeded at 7038.
Hunk #55 succeeded at 7141.
Hunk #56 succeeded at 7243.
Hunk #57 succeeded at 7365.
Hunk #58 succeeded at 7480.
Hunk #59 succeeded at 7627.
Hunk #60 FAILED at 7729.
Hunk #61 succeeded at 7831.
Hunk #62 FAILED at 7934.
Hunk #63 succeeded at 8023.
Hunk #64 succeeded at 8126.
Hunk #65 succeeded at 8230.
Hunk #66 succeeded at 8327.
Hunk #67 succeeded at 8424.
Hunk #68 succeeded at 8540.
Hunk #69 succeeded at 8571.
Hunk #70 succeeded at 8635.
Hunk #71 succeeded at 8976.
Hunk #72 succeeded at 9110.
Hunk #73 succeeded at 9280.
Hunk #74 succeeded at 9455.
Hunk #75 succeeded at 9562.
Hunk #76 succeeded at 9789.
Hunk #77 succeeded at 9904.
Hunk #78 succeeded at 10046.
Hunk #79 succeeded at 10270.
Hunk #80 FAILED at 10371.
Hunk #81 succeeded at 10460.
Hunk #82 succeeded at 10549.
Hunk #83 succeeded at 10646.
Hunk #84 succeeded at 10761.
Hunk #85 succeeded at 10861.
Hunk #86 succeeded at 11005.
Hunk #87 succeeded at 11134.
Hunk #88 succeeded at 11455.
Hunk #89 succeeded at 11591.
Hunk #90 succeeded at 11664.
Hunk #91 succeeded at 11856.
Hunk #92 succeeded at 11933.
Hunk #93 succeeded at 11988.
Hunk #94 succeeded at 12120.
Hunk #95 succeeded at 12196.
Hunk #96 succeeded at 12320.
Hunk #97 succeeded at 12466.
Hunk #98 succeeded at 12509.
Hunk #99 succeeded at 13139.
Hunk #100 succeeded at 13223.
Hunk #101 succeeded at 14235.
Hunk #102 succeeded at 14468.
Hunk #103 succeeded at 15136.
Hunk #104 succeeded at 15345.
Hunk #105 succeeded at 15869.
Hunk #106 succeeded at 15995.
Hunk #107 succeeded at 16107.
Hunk #108 succeeded at 16379.
Hunk #109 succeeded at 16603.
Hunk #110 succeeded at 17172.
Hunk #111 succeeded at 17640.
Hunk #112 succeeded at 18280.
Hunk #113 succeeded at 18305.
Hunk #114 succeeded at 18329.
Hunk #115 succeeded at 18524.
Hunk #116 succeeded at 19029.
Hunk #117 succeeded at 19155.
Hunk #118 succeeded at 19390.
Hunk #119 succeeded at 19525.
Hunk #120 succeeded at 19668.
Hunk #121 succeeded at 19708.
Hunk #122 succeeded at 19897.
Hunk #123 succeeded at 20210.
Hunk #124 succeeded at 20339.
Hunk #125 succeeded at 20953.
Hunk #126 succeeded at 21150.
Hunk #127 succeeded at 21318.
Hunk #128 succeeded at 21748.
Hunk #129 succeeded at 21880.
Hunk #130 succeeded at 22094.
Hunk #131 succeeded at 22198.
Hunk #132 succeeded at 22355.
Hunk #133 succeeded at 22529.
Hunk #134 succeeded at 22758.
Hunk #135 succeeded at 22881.
Hunk #136 succeeded at 23011.
Hunk #137 succeeded at 23150.
Hunk #138 succeeded at 23375.
Hunk #139 succeeded at 23531.
Hunk #140 succeeded at 23670.
Hunk #141 succeeded at 24061.
Hunk #142 succeeded at 24171.
Hunk #143 succeeded at 24653.
Hunk #144 FAILED at 24822.
Hunk #145 succeeded at 25039.
Hunk #146 succeeded at 25201.
Hunk #147 succeeded at 25355.
Hunk #148 succeeded at 25515.
Hunk #149 succeeded at 25648.
Hunk #150 succeeded at 25836.
Hunk #151 succeeded at 25947.
Hunk #152 succeeded at 26095.
Hunk #153 succeeded at 26227.
Hunk #154 succeeded at 26387.
Hunk #155 succeeded at 26513.
Hunk #156 succeeded at 26608.
Hunk #157 succeeded at 26704.
21 out of 157 hunks FAILED -- saving rejects to file resources/slack-openapi-patched.json.rej
done
Error Output:
================
spec:update
Looks good! Thanks a lot! |
First updated the SDK with the latest updates
Then after updating the relevant HTTP methods in
resources/slack-openapi-patched.json
Reference: https://api.slack.com/methods?query=work