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

Use FriendlyNames #459

Closed
wants to merge 12 commits into from
Closed

Use FriendlyNames #459

wants to merge 12 commits into from

Conversation

mmorel-35
Copy link
Contributor

@mmorel-35 mmorel-35 commented Jul 11, 2021

Stores ID and Names for entities in memDB to print friendly names in logs
Fixes #417

@mmorel-35 mmorel-35 requested a review from a team as a code owner July 11, 2021 11:08
@codecov-commenter
Copy link

codecov-commenter commented Jul 11, 2021

Codecov Report

Merging #459 (8543e06) into main (46b074e) will decrease coverage by 0.44%.
The diff coverage is 31.89%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #459      +/-   ##
==========================================
- Coverage   50.29%   49.85%   -0.45%     
==========================================
  Files          72       72              
  Lines        5700     7905    +2205     
==========================================
+ Hits         2867     3941    +1074     
- Misses       2501     3608    +1107     
- Partials      332      356      +24     
Impacted Files Coverage Δ
state/builder.go 0.00% <0.00%> (ø)
state/types.go 71.00% <47.19%> (-2.43%) ⬇️
file/builder.go 55.45% <50.00%> (+3.38%) ⬆️
state/rbac_endpoint_permission.go 71.31% <100.00%> (+0.20%) ⬆️
diff/diff_helpers.go 60.65% <0.00%> (-6.02%) ⬇️
cmd/reset.go 22.50% <0.00%> (-4.92%) ⬇️
cmd/dump.go 21.18% <0.00%> (-4.63%) ⬇️
cmd/convert.go 38.70% <0.00%> (-3.60%) ⬇️
utils/defaulter.go 71.42% <0.00%> (-2.77%) ⬇️
cmd/validate.go 20.00% <0.00%> (-2.59%) ⬇️
... and 66 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 46b074e...8543e06. Read the comment docs.

@mmorel-35 mmorel-35 changed the title Use friendlyNames Use FriendlyNames Jul 11, 2021
@carnei-ro
Copy link
Contributor

Nice features! I just built and tested here and it seems to always present a "diff" adding the property "name".

DECK_MY_VAR=bar /tmp/deck/deck diff -s kong.yml

updating route httpbin  {
   "https_redirect_status_code": 426,
   "id": "ca017d00-c80a-4831-b69c-c975fdef3509",
   "name": "httpbin",
   "path_handling": "v0",
   "paths": [
     "/"
   ],
   "preserve_host": false,
   "protocols": [
     "http",
     "https"
   ],
   "regex_priority": 1,
   "service": {
     "id": "0567f908-7a05-42eb-b53a-a7500c0f7427"
+    "name": "httpbin"
   },
   "strip_path": false
 }

updating plugin pre-function for route httpbin  {
   "config": {
     "access": [
       "return kong.response.exit(200, {["msg"] = "hello from my function" }, {['x-kong-function'] = '[masked]'})
"
     ],
     "body_filter": [
     ],
     "certificate": [
     ],
     "functions": [
     ],
     "header_filter": [
     ],
     "log": [
     ],
     "rewrite": [
     ]
   },
   "enabled": true,
   "id": "49fe5c3a-779a-4a5f-a1b5-f6de9146fc1b",
   "name": "pre-function",
   "protocols": [
     "http",
     "https"
   ],
   "route": {
     "id": "ca017d00-c80a-4831-b69c-c975fdef3509"
+    "name": "httpbin"
   }
 }

Summary:
  Created: 0
  Updated: 2
  Deleted: 0

kong.yml:

---
_format_version: "1.1"

services:
- name: httpbin
  url: https://httpbin.org/anything
  retries: 5
  routes:
  - path_handling: v0
    protocols: 
      - http
      - https
    https_redirect_status_code: 426
    name: httpbin
    paths:
    - /
    preserve_host: false
    regex_priority: 1
    strip_path: false
    plugins:
      - name: pre-function
        enabled: true
        protocols: 
          - http
          - https
        config:
          body_filter: []
          certificate: []
          header_filter: []
          log: []
          rewrite: []
          functions: []
          access:
            - |
              return kong.response.exit(200, {["msg"] = "hello from my function" }, {['x-kong-function'] = '${{ env "DECK_MY_VAR" }}'})

@mmorel-35
Copy link
Contributor Author

I see, the names are not loaded on the dump part. I'm not sure what is right way to do this.
@kong/team-k8s, any idea?

@mmorel-35
Copy link
Contributor Author

I think modifying the state/builder.go to integrate Names shall do the trick

@mmorel-35 mmorel-35 marked this pull request as draft July 20, 2021 05:31
@mmorel-35 mmorel-35 marked this pull request as ready for review July 20, 2021 07:02
@mmorel-35
Copy link
Contributor Author

@carnei-ro would you mind trying this latest version ?

@carnei-ro
Copy link
Contributor

@mmorel-35 It seems to be working as expected!

ChristianJacquot added a commit to ChristianJacquot/deck that referenced this pull request Aug 4, 2021
Inspire by the the PR Kong#459 Use FriendlyNames. Use the ID and the email of a Developer
https://github.com/Kong/deck/pull/459/files
@mmorel-35
Copy link
Contributor Author

Hi @hbagdi @mflendrich , any updates an this pull-request ?

}

func (r *FRoute) asKongRoute() *kong.Route {
return &kong.Route{ID: kong.String(*r.ID), Name: kong.String(*r.Name)}
Copy link
Member

Choose a reason for hiding this comment

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

Name is not a required field for Route or Service.
Please guard dereferences with a nil check. The same applies to consumer because, at runtime, it may happen that the object may not have username (even though it is a required field).

@hbagdi
Copy link
Member

hbagdi commented Nov 19, 2021

This PR is too big and touches different aspects of code - it is very hard to review carefully.
Please break it down into smaller digestible and logical pieces.

There are significant nil exceptions that are written all over the code as well - so please take care of those.

@GGabriele
Copy link
Collaborator

Hi @mmorel-35 , are you still working on this? Any updates since the last request for changes?

@mmorel-35
Copy link
Contributor Author

Hi @GGabriele , I'm sorry I couldn't find the time to work on this lately. I don't know when I'll be able to get back on this so feel free to take it over from now.

@nwsparks
Copy link

nwsparks commented Apr 4, 2022

would be pretty huge to get this available 🙏 the diffs are borderline useless in a lot of cases without this.

@GGabriele
Copy link
Collaborator

@mmorel-35 thanks a LOT for your contribution! I've picked it and merged it in #662

@GGabriele GGabriele closed this May 12, 2022
@mmorel-35
Copy link
Contributor Author

Thank you @GGabriele 👍🏻 !!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Print Names instead of ID when possible
6 participants