-
Notifications
You must be signed in to change notification settings - Fork 352
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
feat: support custom names for generated k8s resources #3537
Conversation
Relates to envoyproxy#3527 Signed-off-by: Arko Dasgupta <[email protected]>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3537 +/- ##
==========================================
+ Coverage 68.16% 68.18% +0.02%
==========================================
Files 167 168 +1
Lines 20370 20408 +38
==========================================
+ Hits 13885 13915 +30
- Misses 5485 5493 +8
Partials 1000 1000 ☔ View full report in Codecov by Sentry. |
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.
probably we would need to also change it in kubernetes provider
func infraName(gateway *gwapiv1.Gateway, merged bool) string { |
hey @cnvergence can you elaborate why thats needed, the above diff is changing the value right before setting the resource name ? |
oh @cnvergence I see the issue now, we are fetching based on name and not labels 😢
&
cc @dprotaso this is the issue seen in knative-extensions/net-gateway-api#738 |
Signed-off-by: Arko Dasgupta <[email protected]>
Signed-off-by: Arko Dasgupta <[email protected]>
Signed-off-by: Arko Dasgupta <[email protected]>
Signed-off-by: Arko Dasgupta <[email protected]>
@@ -228,6 +228,15 @@ func GatewayClassOwnerLabel(name string) map[string]string { | |||
return map[string]string{OwningGatewayClassLabel: name} | |||
} | |||
|
|||
// OwnerLabels returns the owner labels based on the mergeGateways setting | |||
func OwnerLabels(gateway *gwapiv1.Gateway, mergeGateways bool) map[string]string { |
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.
🚀
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.
LGTM
If this is addressed here, will it also fix when I rename the |
Signed-off-by: Arko Dasgupta <[email protected]>
thanks for catching this ! ive fixed the deployment targetRef naming in hpa. |
Signed-off-by: Arko Dasgupta <[email protected]>
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.
🚀
On this same issue. I've renamed the deployment by patching the name, this causes the gateway to break. While the deployment with 3 replicas I get this error Last Transition Time: 2024-06-14T15:02:58Z
Message: The Gateway has been scheduled by Envoy Gateway
Observed Generation: 1
Reason: Accepted
Status: True
Type: Accepted
Last Transition Time: 2024-06-14T15:03:07Z
Message: Deployment replicas unavailable
Observed Generation: 1
Reason: NoResources
Status: False
Type: Programmed the gateway status is this NAME CLASS ADDRESS PROGRAMMED AGE
acme-envoy acme-envoy IPADDRESS False 4m19s |
@davem-git can you raise a separate GH issue to highlight the issue you are facing |
* should have been part of envoyproxy#3537 Signed-off-by: Arko Dasgupta <[email protected]>
Relates to #3527