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

feat(db) move consumers from kong.dao to kong.db #3408

Closed
wants to merge 1 commit into from

Conversation

kikito
Copy link
Member

@kikito kikito commented Apr 23, 2018

Moves consumers to the new DAO. Notes:

  • Includes @bungle's changes to endpoints.lua that allow param parsing using schemas (https://github.com/Kong/kong/compare/feat/admin-schema-aware-parser), so that one should probably be merged before this PR.
  • The consumers admin api has been replaced by auto-generated endpoints, except for plugin-related endpoints:
    • /consumers/:username_or_id/plugins/
    • /consumers/:username_or_id/plugins/:id
  • PUT is gone (for now, @bungle is working on adding it back)
  • "filtering via fields" (/consumers/?created_at=xxx) is no longer possible
  • I am not sure that cache_key should be a DAO method. Maybe it should live in the kong.cache module instead? cache.key('consumers', consumer_id) instead of consumers:cache_key(consumer_id)
  • Notice that I needed to access the old dao from the new db and viceversa, so I have added "shortcuts" to both. It was the only way I found to not use singletons.x

@kikito kikito changed the base branch from master to next April 23, 2018 16:58
Copy link
Member

@bungle bungle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A quick first round of review

DELETE = function(self, dao_factory)
crud.delete(self.consumer, dao_factory.consumers)
end
},

["/consumers/:username_or_id/plugins/"] = {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this be changed to /consumers/:consumers/plugins for future (not also that no / at the end)?


return {
name = "consumers",
primary_key = { "id" },
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

here we need to add endpoint_key = "username", (see feat/admin-schema-aware-parser)

@bungle bungle changed the title Feat/consumers 2 db feat(db) move consumers to db (new model) Apr 26, 2018
@bungle bungle changed the title feat(db) move consumers to db (new model) feat(db) move consumers from kong.dao to kong.db Apr 26, 2018

["/consumers/:username_or_id/plugins/"] = {
["/consumers/:username_or_id/plugins"] = {
Copy link
Member

@bungle bungle May 2, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be /consumers/:consumers/plugins for future changes (or was it not changed because of a reason?), when plugins are transferred (probably at that point this needs to be removed).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have removed the extra "/". Thanks!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also renamed username_or_id to consumers on the route.

Copy link
Member

@bungle bungle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a quick note here. -^

@kikito kikito force-pushed the feat/consumers-2-db branch 2 times, most recently from 536d316 to 211ad2f Compare May 4, 2018 14:05
@kikito
Copy link
Member Author

kikito commented May 7, 2018

Closing this in favour of #3437

@kikito kikito closed this May 7, 2018
@kikito kikito deleted the feat/consumers-2-db branch May 7, 2018 15:21
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.

2 participants