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.

Sign up for GitHub

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

Jump to bottom

fix(admin) disable plugin names in /api/:id/plugins* #2726

Merged
merged 1 commit into from
Jul 25, 2017
Merged

fix(admin) disable plugin names in /api/:id/plugins* #2726

merged 1 commit into from
Jul 25, 2017

Conversation

kikito
Copy link
Member

@kikito kikito commented Jul 24, 2017 •
edited
Loading

@kikito kikito requested a review from Tieske July 24, 2017 21:30
@kikito kikito assigned Tieske and thibaultcha and unassigned Tieske and thibaultcha Jul 24, 2017
@kikito kikito requested a review from thibaultcha July 24, 2017 21:31
@kikito kikito mentioned this pull request Jul 24, 2017
Merged
@kikito kikito force-pushed the fix/admin-api-disable-plugin-name branch from 52a157b to 789c6f7 Compare July 24, 2017 23:20
kikito added a commit that referenced this pull request Jul 24, 2017
@kikito
fix(admin) disable plugin names in /api/:id/plugins*
789c6f7 
* Reverts the main part of #2726
* Needs to be merged before #2714 in order to make its tests pass
@kikito kikito changed the title fix(admin_api) disable using plugin names in /api/{id}/plugins/{id} fix(admin) disable plugin names in /api/:id/plugins* Jul 24, 2017
kikito added a commit that referenced this pull request Jul 24, 2017
@kikito
feat(admin) add /consumers/:id/plugins routes
d47d1a5 
This PR needs #2726 or the api route tests will fail.

Closes #2336
@thibaultcha thibaultcha force-pushed the fix/admin-api-disable-plugin-name branch from 789c6f7 to 2ad1152 Compare July 25, 2017 05:21
@thibaultcha
Copy link
Member

(Slightly reworked this to better follow the conventions of crud_helpers.lua, use the same function on the plugins.lua routes, and to update the commit message with some more context)

thibaultcha
thibaultcha previously approved these changes Jul 25, 2017
View reviewed changes
@thibaultcha thibaultcha added pr/ready This PR is considered ready and can be merged at anytime (given it received no subsequent changes) and removed pr/status/please review labels Jul 25, 2017
@thibaultcha thibaultcha added this to the 0.11.0rc2 milestone Jul 25, 2017
@thibaultcha thibaultcha dismissed their stale review July 25, 2017 07:31

I broke invalidations tests :(

@thibaultcha thibaultcha added pr/status/please review and removed pr/ready This PR is considered ready and can be merged at anytime (given it received no subsequent changes) labels Jul 25, 2017
@thibaultcha
Copy link
Member

Actually it seems that this change in general - and not my latest additions - broke the invalidations tests, since those rely on the plugin by name feature for now.

@kikito
Copy link
Member Author

kikito commented Jul 25, 2017 •
edited
Loading

@thibaultcha I changed the invalidation tests so they didn't rely on the plugin names. I'm leaving that on a separate commit, hoping it is alright.

@thibaultcha
Copy link
Member

@kikito I think it's fine if those are part of the same commit. The scope being "disabling feature X", we should not leave the state of the repo in a non-working state after the related commit is done (harder to detect changes with bisect). Instead, if we remove feature X, we should also update tests relying on feature X at the same time, so that once the commit applied, the repo's tests are left in a working state. I'll be squashing them, no worries!

@thibaultcha thibaultcha force-pushed the fix/admin-api-disable-plugin-name branch from ebc0e1b to 7b2523f Compare July 25, 2017 22:10
@kikito kikito force-pushed the fix/admin-api-disable-plugin-name branch from 7b2523f to 2428275 Compare July 25, 2017 22:16
@kikito @thibaultcha
fix(admin) disable retrieval of plugins by name
8403f3b 
In the `/apis/:api/plugins/:plugin` endpoint, #2252 recently introduced
the possibility to pass a plugin name as `:plugin`, instead of an id
only. Like: `/apis/:api/plugins/basic-auth`.

However, the following minimally reproducible example exposes the flaw
with such an endpoint given the current model:

```
$ http POST :8001/apis/ name=api1 hosts=example.com upstream_url=http://httpbin.org
$ http POST :8001/consumers name=peter
$ http POST :8001/consumers name=mary
$ http POST :8001/plugins name=request-size-limiting api_id=$API1_UUID consumer_id=$PETER_UUID
$ http POST :8001/plugins name=request-size-limiting api_id=$API1_UUID consumer_id=$MARY_UUID
$ http :8001/apis/$API1_UUID/plugins
$ http :8001/apis/$API1_UUID/plugins/request-size-limiting
```

Reverts the main part of #2252

Signed-off-by: Thibault Charbonnier <[email protected]>
@thibaultcha thibaultcha force-pushed the fix/admin-api-disable-plugin-name branch from 2428275 to 8403f3b Compare July 25, 2017 22:43
@thibaultcha thibaultcha added pr/ready This PR is considered ready and can be merged at anytime (given it received no subsequent changes) and removed pr/status/please review labels Jul 25, 2017
@kikito kikito merged commit 38bdb2d into next Jul 25, 2017
@kikito kikito deleted the fix/admin-api-disable-plugin-name branch July 25, 2017 23:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thibaultcha thibaultcha thibaultcha approved these changes

@Tieske Tieske Awaiting requested review from Tieske

Assignees
No one assigned
Labels
pr/ready This PR is considered ready and can be merged at anytime (given it received no subsequent changes)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@kikito @thibaultcha @Tieske