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

Feat/#252 - kumactl get for single entities #667

Merged
merged 30 commits into from
Apr 15, 2020
Merged

Feat/#252 - kumactl get for single entities #667

merged 30 commits into from
Apr 15, 2020

Conversation

tharun208
Copy link
Contributor

@tharun208 tharun208 commented Apr 6, 2020 •
edited
Loading

Summary

kumactl get command for single entities

Full changelog

  • Implemented command to get information about a single entity of Kuma for the following

    • Fault Injection
    • Dataplane
    • Mesh
    • Health Check
    • proxyTemplate
    • trafficLog
    • trafficPermission
    • trafficRoute
    • trafficTrace

  • Added tests

  • Updated tests for kumactl apply

Issues resolved

Fix #252

tharun208 added 2 commits April 6, 2020 23:48
@tharun208
feature(kumactl) added get command for single resources
2c8a5fa 
* added initial files
Feature #252
@tharun208
feature(kumactl) added get command for single resources
e8399bc 
* added validation for single resource
Feature #252
jakubdyszkiewicz
jakubdyszkiewicz reviewed Apr 7, 2020
View reviewed changes
Copy link
Contributor

@jakubdyszkiewicz jakubdyszkiewicz left a comment

Choose a reason for hiding this comment

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

I see that the current implementation suggest that the commands will be

kumactl get resource dataplane dp-1
kumactl get resource mesh mesh-1

I'd rather have the more straightforward commands like

kumactl get dataplane dp-1
kumactl get mesh mesh-1

Sorry if that was your question on Slack and I misunderstood it.

Please start with 2/3 commands (mesh, dataplane, traffic-permission) and let's see what we can do to avoid code duplication.

@tharun208
Copy link
Contributor Author

And we have commands like get meshes, get data planes, can I just create a get mesh mesh-name for now and later, can we think of a better approach ?.

Is this fine because if I start with the get root command to handle args, it will break our existing implementation of underlying sub-commands.

@jakubdyszkiewicz
Copy link
Contributor

Yes, I think the right approach for now is to create a separate sub commands get dataplane, get mesh etc.

tharun208 added 2 commits April 7, 2020 16:15
@tharun208
feature(kumactl) added get command for single resource
3169c9e 
* added get single resource command for mesh
Feature #252
@tharun208
feature(kumactl) added get command for single resource
872170d 
* updated tests
Feature #252
tharun208 added 2 commits April 7, 2020 21:37
@tharun208
feature(kumactl) added get command for single resource
62818d3 
* updated get mesh implementation and tests
Feature #252
@tharun208
feature(kumactl) added get command for single resource
8ffcb9d 
* added get command for dataplane and tests
Feature #252
@tharun208
Copy link
Contributor Author

@jakubdyszkiewicz I have added the implementation for get mesh and get dataplane

@tharun208
Copy link
Contributor Author

I am not finding any possible abstraction between these two. I thought of abstracting out printTable but it should be specific to each resource type.

@jakubdyszkiewicz
Copy link
Contributor

Hey, yes please - reuse the same function for printing table. As you can see there is a difference already, get dataplane does not return tags where as get dataplanes returns tags.

For tests I think we can reuse YAML file for input and just have a one parametrized test https://github.com/Kong/kuma/commit/b4e56abe2bb81ec65dd9136f8a6fee8548175a90

tharun208 added 12 commits April 8, 2020 17:30
@tharun208
Merge pull request #1 from Kong/get-cmd
7a1c4c5 
generic test
@tharun208
feature(kumactl) added get command for single resource
dcdc6e7 
* resused printDataplanes for printing output
Feature #252
@tharun208
feature(kumactl) added get command for single resource
e93ab12 
* made changes in printdataplanes to support both get dataplane
Feature #252
@tharun208
feature(kumactl) added get command for single resource
de1a42e 
* added get fault injection and refractored printFaultInjection
Feature #252
@tharun208
feature(kumactl) added get command for single resource
b6b4aeb 
* formatted get-mesh yaml
Feature #252
@tharun208
feature(kumactl) added get command for single resource
fbba3cd 
* added get-healthcheck command
* changed types of printHealthCheck to support for single and multi resource
Feature #252
@tharun208
feature(kumactl) added get command for single resource
5fc3a5c 
* added get proxytemplate command
* changed types of PrintProxyTemplate to support for single and multi resource
Feature #252
@tharun208
feature(kumactl) added get command for single resource
cdc4320 
* renamed get_fault_injection file
Feature #252
@tharun208
feature(kumactl) added get command for single resource
3d3bd6c 
* added get command to get single traffic-log
* refractored printTrafficLog
* added tests
@tharun208
feature(kumactl) added get command for single resource
817be60 
* reverted back checking mesh type in apply cmd
Feature #252
@tharun208
feature(kumactl) added get command for single resource
fd7e49d 
* commenting out mesh in test
Feature #252
@tharun208
feature(kumactl) added get command for single resource
763917f 
* Fixed failing tests
Feature #252
@tharun208 tharun208 marked this pull request as ready for review April 8, 2020 20:12
@tharun208 tharun208 requested a review from a team April 8, 2020 20:12
jakubdyszkiewicz
jakubdyszkiewicz reviewed Apr 10, 2020
View reviewed changes
Copy link
Contributor

@jakubdyszkiewicz jakubdyszkiewicz left a comment

Choose a reason for hiding this comment

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

Thanks! That's awesome progress. I think there are still some resources left to be added

app/kumactl/cmd/get/get_fault_injections.go Outdated Show resolved Hide resolved
app/kumactl/cmd/get/get_fault_injections.go Outdated Show resolved Hide resolved
app/kumactl/cmd/get/get_proxytemplates.go Outdated Show resolved Hide resolved
app/kumactl/cmd/get/get_traffic_logs.go Outdated Show resolved Hide resolved
app/kumactl/cmd/get/testdata/get-mesh.golden.yaml Show resolved Hide resolved
@tharun208
feature(kumactl) added get command for single resource
e0b668f 
* Fixed review feedbacks
Feature #252
@tharun208
feature(kumactl) added get command for single resource
1328221 
* Added get traffic permission to get single traffic permission resource
Feature #252
@jakubdyszkiewicz jakubdyszkiewicz mentioned this pull request Apr 10, 2020
Merged
@tharun208
feature(kumactl) added get command for single resource
6250cf1 
* Added get traffic route to get single traffic route resource
Feature #252
@tharun208
feature(kumactl) added get command for single resource
8349fd4 
* Added get traffic trace to get single traffic trace resource
Feature #252
@tharun208
feature(kumactl) added get command for single resource
6214080 
* made printProxyTemplates as private function
Feature #252
@tharun208
feature(kumactl) added get command for single resource
786ad24 
* fixed import issue
Feature #252
jakubdyszkiewicz
jakubdyszkiewicz reviewed Apr 10, 2020
View reviewed changes
Copy link
Contributor

@jakubdyszkiewicz jakubdyszkiewicz left a comment

Choose a reason for hiding this comment

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

almost there, can you add a changelog?

app/kumactl/cmd/get/testdata/get-traffic-route.golden.yaml Show resolved Hide resolved
app/kumactl/cmd/get/testdata/get-proxytemplate.golden.yaml Show resolved Hide resolved
app/kumactl/cmd/get/testdata/get-healthcheck.golden.yaml Show resolved Hide resolved
@tharun208
feature(kumactl) added get command for single resource
89ed592 
* added detailed configuration for healthCheck, proxyTemplate,TrafficRoute
Feature #252
@tharun208
feature(kumactl) added get command for single resource
9cfcd66 
* added changelog
Feature #252
@tharun208
feature(kumactl) added get command for single resource
c322ccf 
* added changelog
Feature #252
jakubdyszkiewicz
jakubdyszkiewicz reviewed Apr 14, 2020
View reviewed changes
Copy link
Contributor

@jakubdyszkiewicz jakubdyszkiewicz left a comment

Choose a reason for hiding this comment

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

It would help a lot with the review if the commits were more meaningful instead of same name for every one of them. Thanks!

app/kumactl/cmd/get/testdata/get-fault-injection.golden.yaml Outdated Show resolved Hide resolved
app/kumactl/cmd/get/get_dataplane.go Outdated Show resolved Hide resolved
@tharun208
Copy link
Contributor Author

It would help a lot with the review if the commits were more meaningful instead of same name for every one of them. Thanks!

sure noted it

@jakubdyszkiewicz jakubdyszkiewicz merged commit 6cccc6b into kumahq:master Apr 15, 2020
@jakubdyszkiewicz
Copy link
Contributor

Thank you for working on this!

@jakubdyszkiewicz jakubdyszkiewicz mentioned this pull request Apr 15, 2020
Closed
6 tasks
@tharun208 tharun208 deleted the feat/#252 branch April 17, 2020 18:10
@tharun208 tharun208 mentioned this pull request May 13, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lobkovilya lobkovilya lobkovilya left review comments

@jakubdyszkiewicz jakubdyszkiewicz jakubdyszkiewicz approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Introduced describe as a new CLI command for our entities
3 participants
@tharun208 @jakubdyszkiewicz @lobkovilya