-
Notifications
You must be signed in to change notification settings - Fork 19
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
Add k8s resources types #199
Conversation
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.
You will either need to:
- change your local go env to 1.18 and run rake
- wait/approve my codegen fix: [rake] add gimme for default task #201 and re-run
Nitpick but the indentation seems off. Also is there any reason they're all in caps? |
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.
Can still fix some styling but overall looks good to me.
proto/process/agent.proto
Outdated
unsetNodeType = 0; | ||
pod = 1; | ||
replicaSet = 2; | ||
service = 3; | ||
node = 4; | ||
cluster = 5; | ||
job = 6; | ||
cronJob = 7; | ||
daemonSet = 8; | ||
statefulSet = 9; | ||
persistentVolume = 10; | ||
persistentVolumeClaim = 11; | ||
role = 12; | ||
roleBinding = 13; | ||
clusterRole = 14; | ||
clusterRoleBinding = 15; | ||
serviceAccount = 16; | ||
ingress = 17; | ||
deployment = 18; | ||
namespace = 19; | ||
crd = 20; | ||
cr = 21; |
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.
several thoughts:
- it would make more sense to use the singular for that enum name i.e.
K8sResource
- we should use upper case format for values here as specified in https://developers.google.com/protocol-buffers/docs/style#enums
- by convention the first value should have an
UNSPECIFIED
prefix like inK8S_RESOURCE_UNSPECIFIED
, is it possible to rename this or is it annoying? unfortunately looks like we started using "unset" here and there for some reason 😕
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.
Thanks for the feedback, I addressed in c440357
I don't add prefix in proto, such as K8SRESOURCE_POD
, because in generated file we can have K8sResource_K8SRESOURCE_POD
making me feel the name is too long
Add k8s resources type so it can be used by both agent and backend side
Agent PR DataDog/datadog-agent#14557