-
Notifications
You must be signed in to change notification settings - Fork 0
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: added support for degraphql_routes #154
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #154 +/- ##
==========================================
- Coverage 27.95% 27.27% -0.69%
==========================================
Files 106 109 +3
Lines 16358 16967 +609
==========================================
+ Hits 4573 4627 +54
- Misses 11303 11851 +548
- Partials 482 489 +7 ☔ View full report in Codecov by Sentry. |
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.
From a code perspective, this looks good to me, except for a few nits.
I have a few comments from a product perspective though. I see we are following what the KIC team is doing, but:
- we are calling this
plugin_entities
, but there is nothing related to plugins inside each entity. I understand the reasons because I know what's behind the scenes, but I think users may be confused by the naming - still on the naming, we usually identify as an
entity
as a single row of a given type. Now sayingI have 10 plugin entities
sounds a bit ambiguous: do you have 10plugins
or do you have 10plugin_entities
? plugin_entities
is not DB-less compatible. I know decK is not fully db-less compatible because some differences exist onconsumer_groups
, but these differences have been source of friction for our users. Do we intend to further expand on this friction?
pkg/state/builder.go
Outdated
var degraphqlRoute DegraphqlRoute | ||
|
||
if entity["id"] != nil { | ||
degraphqlRoute.ID = kong.String(entity["id"].(string)) |
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.
In this function, please handle cases in which the type assertions fail. We never know what users can include in their files and failing with a graceful error is always desirable
So, from
DB-less mode identifies cc: @mheap |
I hadn't considered db-less compatibility. This is a very good callout. How much work would it be to change the format to match DB-less? |
@mheap As of now, I had considered a common yaml format for all plugin entities and the code was doing the parsing, processing and everything else accordingly. Only the API calls were made to degraphql_routes. DB-less identifies "degraphql_routes" as a separate entity. So, I would have to rewrite a bunch of things to match it to DB-less. This would also defeat the purpose of using a common way and yaml format to handle all plugin entities. |
@hbagdi What are your thoughts here? I'd rather not increase the gap between DB-less and Deck formats, but I think this is an instance where we can provide a nicer UX + minimise maintenance cost |
Let's proceed with the current design rather than mirroring the DB-less format |
@GGabriele I have added the suggestions you mentioned as a part of the first review. |
2900111
to
a01aa3f
Compare
Summary
Adding support for degraphql_routes so that
they can be managed with decK.
Tests added in separate PR: #155
Working on integration tests in the same PR.
YAML representation:
The above representation is chosen so that other custom-entities can be added in the same way, without changes to parsing/dumping logic. In the code,
FPluginEntity
is used as a superset type that can hold different custom entities.Issues resolved
For Kong/deck#1346
Documentation
Testing