Use ServiceID to match VPC Endpoint Service in ReadMany operation #273
Conversation
|
Skipping CI for Draft Pull Request. |
ack-prow
bot
added
the
do-not-merge/work-in-progress
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: knottnt The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
| build_date: "2025-07-08T05:50:37Z" | ||
| build_hash: 580a56e85fed07f60089ef2cae634548b523981b | ||
| go_version: go1.24.3 | ||
| version: v0.48.0-2-g580a56e-dirty |
There was a problem hiding this comment.
Needs to be re-built once this code-gen PR is merged.
| @@ -254,12 +254,6 @@ operations: | |||
| operation_type: | |||
| - Delete | |||
| resource_name: VpcEndpointServiceConfiguration | |||
| CreateVpcEndpointServiceConfiguration: | |||
There was a problem hiding this comment.
Removed duplicate operation objects
| acktest @ git+https://github.com/knottnt/ack-test-infra.git@93adae31a050a11a67567e1e8d3c440b47f4044e |
There was a problem hiding this comment.
Need to replace this once this PR is merged
| if ko.Status.AllocationID == nil || *elem.AllocationId != *ko.Status.AllocationID { | ||
| continue |
There was a problem hiding this comment.
It looks like Elastic IP Address had a similar issue which is solved with this hook. It has the same affect of causing the nil Status field to result in a Not Found Error. While it does require custom code it does have the benefit of avoiding the ReadMany API call and loop which could have performance benefits when dealing with a large amount of AWS resources.
I'd be interested in you thoughts on this trade off of moving custom code to code-generation vs performance in high resource count environments. Alternatively, we could look at improving my code-generator change to add a check similar to the hook.
There was a problem hiding this comment.
Note, this degrades the performance to that of a Spec matching field where no match exists and we can't specify a filter in the ReadMany API call.
|
/test all |
|
/retest |
knottnt
force-pushed
the
endpoint-service-matching
branch
from
640a2ad to
2251ee3
Compare
|
/retest |
knottnt
force-pushed
the
endpoint-service-matching
branch
from
2251ee3 to
22a4b21
Compare
|
/retest |
1 similar comment
|
/retest |
knottnt
force-pushed
the
endpoint-service-matching
branch
from
22a4b21 to
73e0514
Compare
|
/retest |
ack-prow
bot
removed
the
do-not-merge/work-in-progress
- Use ServiceID as matching field for list operation - Update synced check to use Available instead of available
knottnt
force-pushed
the
endpoint-service-matching
branch
from
73e0514 to
4ea0400
Compare
| @@ -1068,7 +1062,10 @@ resources: | |||
| when: | |||
| - path: Status.ServiceState | |||
| in: | |||
| - available | |||
| - Available | |||
There was a problem hiding this comment.
Resource was never reached ACK.ResourceSynced=True due to case mismatch.
|
@knottnt: The following test failed, say
Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
Issue #, if available:
Description of changes:
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.