Skip to content

Fix late init of SecurityGroupRule with self: true - #229

Merged
ytsarev merged 1 commit into
crossplane-contrib:mainfrom
ytsarev:sg-rule-self
Dec 5, 2022
Merged

Fix late init of SecurityGroupRule with self: true#229
ytsarev merged 1 commit into
crossplane-contrib:mainfrom
ytsarev:sg-rule-self

Conversation

@ytsarev

@ytsarev ytsarev commented Dec 2, 2022

Copy link
Copy Markdown
Contributor

Description of your changes

Add late init to self and source_security_group_id

Fixes #228

Signed-off-by: Yury Tsarev yury@upbound.io

I have:

  • Run make reviewable test to ensure this PR is ready for review.

How has this code been tested

k apply -f examples/ec2/securitygrouprule-self-true.yaml
k get managed
NAME                                        READY   SYNCED   EXTERNAL-NAME          AGE
securitygroup.ec2.aws.upbound.io/test-vpc   True    True     sg-00c2b14d55b3da787   10m

NAME                                READY   SYNCED   EXTERNAL-NAME           AGE
vpc.ec2.aws.upbound.io/sample-vpc   True    True     vpc-0f8da654a40cb68cb   10m

NAME                                                          READY   SYNCED   EXTERNAL-NAME       AGE
securitygrouprule.ec2.aws.upbound.io/test-vpc-securitygroup   True    True     sgrule-2687506433   10m

Fixes crossplane-contrib#228

Signed-off-by: Yury Tsarev <yury@upbound.io>
@ytsarev
ytsarev requested a review from muvaf December 2, 2022 20:59
@ytsarev

ytsarev commented Dec 2, 2022

Copy link
Copy Markdown
Contributor Author

/test-examples="examples/ec2/securitygrouprule-self-true.yaml"

@ulucinar ulucinar left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @ytsarev,
Thank you very much for the quick fix.

As a future work, we can discuss how to utilize the schema.Schema.ConflictsWith to auto configure late initialization.

Comment thread config/ec2/config.go
}
r.LateInitializer = config.LateInitializer{
IgnoredFields: []string{
"self", "source_security_group_id",

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we also include cidr_blocks and ipv6_cidr_blocks together with the source_security_group_id?

I guess they don't get late-initialized since we have already tested the new configuration and hence not included here. Is this correct?

@@ -0,0 +1,44 @@
apiVersion: ec2.aws.upbound.io/v1beta1

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We had better add the meta.upbound.io/example-id: ec2/v1beta1/securitygrouprule annotations to the resource manifests.

@ytsarev
ytsarev merged commit 3d2b6e2 into crossplane-contrib:main Dec 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ec2: SecurityGroupRule is not working when spec.forProvider.self is set to true

2 participants