✨ Add test for provider-managed infrastructure, fix writing latest status back into ClusterScope
#4637
Conversation
k8s-ci-robot
added
release-note-none
k8s-ci-robot
added
the
size/XL
api/v1beta2/network_types.go
Outdated
| if x.GetResourceID() == id { | ||
| return &x | ||
| return &(*s)[i] // pointer to original structure |
There was a problem hiding this comment.
For readability, can we allocate x after the range above and then use that?
There was a problem hiding this comment.
Done – it's now consistent with the other pointer uses of x 👍
| // Sort so that unit tests can expect a stable order | ||
| sort.Slice(tags, func(i, j int) bool { return *tags[i].Key < *tags[j].Key }) |
There was a problem hiding this comment.
Could we make Tags an alias sliced and satisfy the sort interface instead of copying this function over?
There was a problem hiding this comment.
There was a problem hiding this comment.
Do you mean something like this?
type EC2Tags []*ec2.Tag
func (a EC2Tags) Len() int {
return len(a)
}
func (a EC2Tags) Less(i, j int) bool {
if a[i].Key == nil {
return a[j].Key != nil
} else if a[j].Key == nil {
return false
}
return *a[i].Key < *a[j].Key
}
func (a EC2Tags) Swap(i, j int) {
a[i], a[j] = a[j], a[i]
}
// MapToTags converts a infrav1.Tags to a []*ec2.Tag.
func MapToTags(src infrav1.Tags) EC2Tags {
tags := make(EC2Tags, 0, len(src))
for k, v := range src {
tag := &ec2.Tag{
Key: aws.String(k),
Value: aws.String(v),
}
tags = append(tags, tag)
}
// Sort so that unit tests can expect a stable order
sort.Sort(tags)
return tags
}
We'd have to do this for every type i.e. IAM, EC2, etc. That also probably means changing interfaces from []*ec2.Tag to our alias.
Alternative suggestion: avoid repetition with slices.SortFunc(tags, SortByKey) and implement the SortByKey function once.
There was a problem hiding this comment.
I like the SortByKey function idea; it saves on a bigger refactoring.
However, I do think that revisiting these functions and types for the Tags makes sense.
There was a problem hiding this comment.
slices.SortFunc won't work because Go generics do not yet support accessing common fields.
Can we leave this for another issue since it blows up the scope of this PR and the problem is also in existing code?
api/v1beta2/network_types.go
Outdated
| if (spec.GetResourceID() != "" && x.GetResourceID() == spec.GetResourceID()) || | ||
| (spec.CidrBlock == x.CidrBlock) || | ||
| (spec.IPv6CidrBlock != "" && spec.IPv6CidrBlock == x.IPv6CidrBlock) { | ||
| return &x | ||
| return &(*s)[i] // pointer to original structure |
There was a problem hiding this comment.
Same comment as @vincepri mentioned above
| // Sort so that unit tests can expect a stable order | ||
| sort.Slice(tags, func(i, j int) bool { return *tags[i].Key < *tags[j].Key }) |
There was a problem hiding this comment.
AndiDog
force-pushed
the
managed-subnet-test
branch
from
b9381c1 to
6b52231
Compare
AndiDog
changed the title
ClusterScopeClusterScope
AndiDog
force-pushed
the
managed-subnet-test
branch
from
6b52231 to
987df24
Compare
|
/milestone v2.4.0 |
|
/test pull-cluster-api-provider-aws-apidiff-main |
AndiDog
force-pushed
the
managed-subnet-test
branch
2 times, most recently
from
93206d3 to
e12117f
Compare
|
We could make Key a getter function instead?
…On Mon, Feb 5, 2024 at 8:58 PM Andreas Sommer ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In pkg/cloud/converters/tags.go
<#4637 (comment)>
:
> + // Sort so that unit tests can expect a stable order
+ sort.Slice(tags, func(i, j int) bool { return *tags[i].Key < *tags[j].Key })
slices.SortFunc won't work because Go generics do not yet support
accessing common fields.
Can we leave this for another issue since it blows up the scope of this PR
and the problem is also in existing code?
—
Reply to this email directly, view it on GitHub
<#4637 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAXZJ73J3YER4JA55VB7ZFTYSGZ7ZAVCNFSM6AAAAAA7M3UAXKVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTQNRUGI3TSMZQGU>
.
You are receiving this because you were mentioned.Message ID:
<kubernetes-sigs/cluster-api-provider-aws/pull/4637/review/1864279305@
github.com>
|
Yes, I had the same thought but then the types must be changed, I think (as in my sorting interface idea). Still, this increases the scope of this PR which is targeted for this week's release milestone. I'd rather follow up in a separate PR to make that cleaner. |
|
/lgtm I'm ok with deferring the sorting to a different PR. |
k8s-ci-robot
added
the
lgtm
api/v1beta2/network_types.go
Outdated
| // Deprecated: This function makes a copy, so the returned pointer cannot be used to write back into the original | ||
| // slice. Please use FindByIDPtr. |
There was a problem hiding this comment.
Hm, s Subnets is a slice, which is generally already a pointer internally. Could we modify this function to return the pointer to the struct within the slice?
There was a problem hiding this comment.
func (s Subnets) FindByID(id string) *SubnetSpec {
for i := 0; i < len(s); i++ {
x := &(s[i]) // pointer to original structure
if x.GetResourceID() == id {
return x
}
}
return nil
}This should do?
There was a problem hiding this comment.
There was a problem hiding this comment.
TIL – that works. I pushed this change.
AndiDog
force-pushed
the
managed-subnet-test
branch
from
e12117f to
99fb707
Compare
k8s-ci-robot
removed
the
lgtm
|
/lgtm |
k8s-ci-robot
added
lgtm
|
I updated the release note. Thanks @AndiDog for this. /approve |
|
Until the e2e have finished running /hold |
k8s-ci-robot
added
the
do-not-merge/hold
|
/test ? |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: richardcase 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 |
k8s-ci-robot
added
the
approved
|
@richardcase: The following commands are available to trigger required jobs:
The following commands are available to trigger optional jobs:
Use
In response to this:
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. |
|
/test pull-cluster-api-provider-aws-e2e |
|
The e2e passed so: /unhold |
k8s-ci-robot
removed
the
do-not-merge/hold
|
Meh, that failure was for an old commit 🤷. Running it again on latest commit. /test pull-cluster-api-provider-aws-test |
|
Ah no, it's based on a merge and actually failing. I need to fix this and rebase once more. Give me a few minutes. |
…us back into `ClusterScope`
AndiDog
force-pushed
the
managed-subnet-test
branch
from
99fb707 to
3e62fbf
Compare
k8s-ci-robot
removed
the
lgtm
|
/test pull-cluster-api-provider-aws-test |
|
/test pull-cluster-api-provider-aws-verify |
|
Belt and braces again: /test pull-cluster-api-provider-aws-e2e |
|
/lgtm |
k8s-ci-robot
added
the
lgtm
What type of PR is this?
/kind feature
What this PR does / why we need it:
In #4474, @vincepri re-introduced the managed-subnet functionality. This PR adds a test to ensure that we don't lose the functionality again by mistake.
This is a forward-port of the test which I had originally added in our CAPA fork (giantswarm@407bd98). Back then, I found an issue where updated information wasn't written back to the scope because
func (s Subnets) Find[...] *SubnetSpecreturned a pointer to a copy. I left this change in here – let me know if it should be split out.Which issue(s) this PR fixes:
Adds a test for #4026
Checklist:
Release note: