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

traffic permission entity #126

Merged
merged 3 commits into from
Aug 27, 2019
Merged

traffic permission entity #126

merged 3 commits into from
Aug 27, 2019

Conversation

gszr
Copy link
Contributor

@gszr gszr commented Aug 22, 2019 •
edited

Summary

  • Add TrafficPermission entity
  • Add konvoyctl get traffic-permissions

jakubdyszkiewicz
jakubdyszkiewicz requested changes Aug 22, 2019
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.

Delete validations files. We don't need those.
Remember to generate CRD for K8S.

components/konvoy-control-plane/api/mesh/v1alpha1/traffic_permission.proto Outdated Show resolved Hide resolved
components/konvoy-control-plane/api/mesh/v1alpha1/traffic_permission.proto Outdated Show resolved Hide resolved
@gszr gszr force-pushed the feature/traffic-permission-entity branch 2 times, most recently from 86a8073 to e612a99 Compare August 22, 2019 18:39
@gszr gszr changed the title wip: traffic permission entity traffic permission entity Aug 22, 2019
@gszr gszr requested a review from a team August 22, 2019 19:11
@gszr gszr force-pushed the feature/traffic-permission-entity branch 3 times, most recently from 0822844 to 0f21b1f Compare August 22, 2019 22:12
yskopets
yskopets suggested changes Aug 22, 2019
View reviewed changes
components/konvoy-control-plane/api/mesh/v1alpha1/traffic_permission.proto Outdated Show resolved Hide resolved
components/konvoy-control-plane/api/mesh/v1alpha1/traffic_permission.proto Outdated Show resolved Hide resolved
components/konvoy-control-plane/api/mesh/v1alpha1/traffic_permission.proto Outdated Show resolved Hide resolved
components/konvoy-control-plane/api/mesh/v1alpha1/traffic_permission.proto
@@ -0,0 +1,19 @@
syntax = "proto3";
Copy link
Contributor

Choose a reason for hiding this comment

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

I would also suggest to create a unit test that unmarshals example YAML into a Golang struct. To ensure that Protobuf definition implements requirements of konvoy-docs.

E.g., https://github.com/Kong/konvoy/blob/master/components/konvoy-control-plane/api/mesh/v1alpha1/dataplane_test.go#L14

components/konvoy-control-plane/app/konvoyctl/cmd/get/get_trafficpermissions.go Outdated Show resolved Hide resolved
...e/pkg/plugins/resources/k8s/native/config/crd/bases/mesh.getkonvoy.io_trafficpermission.yaml Outdated Show resolved Hide resolved
...e/app/konvoyctl/data/install/k8s/control-plane/crds/mesh.getkonvoy.io_trafficpermission.yaml Outdated Show resolved Hide resolved
...e/app/konvoyctl/data/install/k8s/control-plane/crds/mesh.getkonvoy.io_trafficpermission.yaml Outdated Show resolved Hide resolved
...e/app/konvoyctl/data/install/k8s/control-plane/crds/mesh.getkonvoy.io_trafficpermission.yaml Outdated Show resolved Hide resolved
components/konvoy-control-plane/app/konvoyctl/cmd/get/get_trafficpermissions.go Show resolved Hide resolved
@gszr gszr force-pushed the feature/traffic-permission-entity branch 2 times, most recently from e66102c to 3b7a62e Compare August 25, 2019 17:53
yskopets
yskopets reviewed Aug 26, 2019
View reviewed changes
Copy link
Contributor

@yskopets yskopets left a comment

Choose a reason for hiding this comment

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

yskopets
TrafficPermission will not have status field

gszr
Done; could you elaborate on why though?

status field is a very Kubernetes-specific construct, while we're want to avoid similarities with Kubernetes

...ents/konvoy-control-plane/dev/examples/universal/trafficpermissions/trafficpermission-2.yaml Outdated Show resolved Hide resolved
...-control-plane/app/konvoyctl/cmd/install/testdata/install-control-plane.defaults.golden.yaml Show resolved Hide resolved
@gszr gszr force-pushed the feature/traffic-permission-entity branch 3 times, most recently from ae13ea8 to 91d5ff5 Compare August 26, 2019 22:48
@gszr gszr force-pushed the feature/traffic-permission-entity branch from 91d5ff5 to 58ebec4 Compare August 27, 2019 16:21
@gszr gszr requested a review from yskopets August 27, 2019 16:36
@gszr gszr force-pushed the feature/traffic-permission-entity branch from 58ebec4 to 080f0b6 Compare August 27, 2019 16:49
@gszr gszr merged commit 336acde into master Aug 27, 2019
@gszr gszr deleted the feature/traffic-permission-entity branch August 27, 2019 17:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jakubdyszkiewicz jakubdyszkiewicz jakubdyszkiewicz requested changes

@yskopets yskopets yskopets 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.

None yet
3 participants
@gszr @jakubdyszkiewicz @yskopets