-
Notifications
You must be signed in to change notification settings - Fork 49
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 Tagging to lattice target group resource #59
Conversation
createTargetGroupInput.Tags[latticemodel.K8SIsServiceExportKey] = &value | ||
} else { | ||
value := latticemodel.K8SIsNotServiceExport | ||
createTargetGroupInput.Tags[latticemodel.K8SIsServiceExportKey] = &value |
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.
nit: i prefer string types instead of bool types for extensibility(in case targetGroup needed for other resources)
e.g. Tags[resourceType] = "serviceExport"
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.
Further, you can use a library to pointer-ify these for you inline: lo.ToPtr(string) or aws.String(string)
IsServiceExport bool `json:"serviceexport"` | ||
K8SServiceName string `json:"k8sservice"` | ||
K8SServiceNamespace string `json:"k8sservicenamespace"` | ||
K8SHTTPRouteName string `json:"k8shttproutename"` | ||
K8SHTTPRouteNamespace string `json:"k8shttproutenamespace"` |
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.
Am I correct in thinking that this is a user facing API?
- We shouldn't be using
k8s
in the api field name - The field should use camelcase
- Instead of hardcoding name and namespace, we should use something like this
ref:
kind: HTTPRoute
name: myname
namespace: mynamespace
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.
these fields are internal.
pkg/model/lattice/targetgroup.go
Outdated
K8SIsServiceExport = "true" | ||
K8SIsNotServiceExport = "false" |
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.
K8SIsServiceExport = "true" | |
K8SIsNotServiceExport = "false" | |
TrueString = "true" | |
FalseString = "false" |
Issue #, if available:
Description of changes:
Today, gateway controller create a lattice target group if a K8S service is referenced by K8S
serviceexport
object or referenced by a backendref of a HTTPRoute.By adding the tags, gateway controller can track which
serviceexport
andhttproute
triggered creation of target group. And gateway controller can use it to delete unused target groups #15By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.